Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379
Performance Enhancement :Enable Passing of requestPath to CRT and monitor the progress using CRTs S3MetaRequestProgress #4379
Changes from 1 commit
0e28441
35fec51
144c10b
66467b5
6af3397
65ddbca
137862e
99b7736
aaa43a0
0b88836
cce960e
152d896
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CRT is reading directly from disk, does this publisher get used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRT directly gets this from file. The Publisher here is in the S3Client Interface and a non null is required just to get the content length a I have updated the code to throw an exception if we try to read the Java SDKs Publisher in such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a weird application of polymorphism. Instead of S3CrtProgressListener, can we just have a NoOpPublisherListener that we create if delegateListener is null? That just means we need the null check in the setup code of S3CrtResponseHandlerAdapter instead of per-method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created NoOpPublisherListener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to invoke
this.progressListener.subscriberOnError();
inonErrorResponseComplete
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already calling
this.progressListener.subscriberOnError(exception);
fromfailResponseHandlerAndFuture
which is called fromerrorPayload != null
alsoerrorPayload == null
for valid errorPayloadsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failResponseHandlerAndFuture
doesn't seem to be invoked fromonErrorResponseComplete
if it successfully marshalls the error response though. Can we add a test to verify it? This test may not suffice now that we are relying on the real CRT client to report progress and it is using a mock s3 client.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new mock tests and removed explicit onError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming nitpicks, since this actually seems like a protected API.
SOURCE
- Do we ever expect to use this for something other than a PutObject - like a GetObject, where it's actually theDESTINATION
?REQ
- We should almost always use the full word for something instead of shortening it, soREQUEST
. It looks like a lot of the existing properties are request properties, though, so I don't think REQUEST is necessary.If it's always the source, what about
PUT_OBJECT_PATH
?UPLOAD_PATH
?If it's potentially also destination, what about
UPLOAD_DOWNLOAD_PATH
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matt..
Named it as OBJECT_FILE_PATH so that it can be used for any file path for get/put
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is marked as @SdkInternalApi, so it can't be used by the transfer manager module, like we're doing above. Do we need to move a subset to a @SdkProtectedApi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done