-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Clarify the semantics of the BlobContainer interface #18157
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
Conversation
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.
can you use expectThrows instead?
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.
(there seems to be a consensus around using expectThrows rather than this try/fail/catch/assert pattern for new tests)
8afed8a to
4a7e321
Compare
|
@jpountz thank you for the feedback! I made the changes, and am catching |
4a7e321 to
e7e3d4a
Compare
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.
Is it worth mentioning? This would be true for any Java code?
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.
I'm not in favour of removing the blobSize information. It can be very useful for implementations to know in advance the size of the blob to be written. For example, the impl could decide to store small blobs differently from large ones... Some clients could also use this info when uploading the blob to a remote storage service like S3/Azure/GCS. Depending of the size of the blob, it can be uploaded in 1 request and large ones could be sent in multiple multi-part requests.
Also, this is required by the upcoming #13578.
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.
@tlrx that's good to know, I didn't realize it was needed by the Google storage plugin. I'll add it back.
e7e3d4a to
dda8ea1
Compare
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.
what if there is an error? should it just return false or throw an unchecked exception?
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.
@rmuir good catch! I'm updating it to reflect that an IOException should be thrown if an error occurs while trying to ascertain the existence of the named blob.
|
Thanks for documenting this! It is much better. I added some questions for potential followup improvements of this stuff. But the contract is now way more clear: +1 |
c6520fb to
c3c5931
Compare
This commit clarifies the behavior that must be adhered to by any implementors of the BlobContainer interface. This is done through expanded Javadocs. Closes elastic#18157 Closes elastic#15580
c3c5931 to
042ad83
Compare
|
@rmuir do you think any other changes are required or is the PR in good shape now? (combined with the github issues created to address the outstanding issues) |
|
sorry i missed your ping, I was +1 8 days ago for these changes! Thanks for opening followups |
This commit clarifies the behavior that must be adhered to by any implementors of the BlobContainer interface. This is done through expanded Javadocs.
Closes #15580