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
SAMZA-2657: Blob Store as backend for Samza State backup and restore #1501
SAMZA-2657: Blob Store as backend for Samza State backup and restore #1501
Changes from all commits
beeeb20
bc74c9c
26abce2
0b4a733
4bf1a45
a2d18f8
f73cc4d
628b3d3
4766b81
334f32f
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.
Docs on what this metadata is for? is BlobMetadata a better name, Metadata seems too generic
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.
No Preconditions check 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.
If this is used to abstract out the specific blob store client, we need to make sure that on contract for the blob store impl must follow wrap this (make sure it is documented in the interface class)
Also, how do we know if it is deleted vs missing/not present? Would the behaviour for MissingException be different?
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.
An instance of this class is passed to the called from Ambry specific impl of blob store manager, if a blob is was explicitly deleted (Ambry returns 410 for this event) and not just missing blob. For other managers, they can implement their own logic to handle this kind of error. Do you think we should let other impls of BlobStoreManager decide how to use/handle this or do you think I should document something here, since this will be dependent on the service.
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.
Should this not extend SamzaException
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.
Follow Up: Document that blob store managers should use these exceptions for the relevant errors, because the blob store agnostic error handling and retry logic in backup and restore managers depends on these. RetriableException is obvious. For DeletedException, document what the error-handling behavior is (e.g. ignores deleted blobs during initial cleanup / snapshot index read etc.)
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.
P2: We would need to document in both BlobStoreManager as well as in this class exactly the expectation of how this class would be used in the BlobStoreManager so that implementers of that and this interface could have that background
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.
+1 to Dan's comment - let's add this to at-throws sections in the BlobStoreManager javadocs instead. Also rephrase: this exception should not be "thrown" synchronously by the manager. Futures should be completed exceptionally with this exception on failure.