-
Notifications
You must be signed in to change notification settings - Fork 275
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
Add undelete method to BlobStore #1373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1373 +/- ##
============================================
+ Coverage 72.01% 72.06% +0.05%
- Complexity 6779 6791 +12
============================================
Files 488 488
Lines 38445 38504 +59
Branches 4890 4898 +8
============================================
+ Hits 27685 27747 +62
Misses 9421 9421
+ Partials 1339 1336 -3
Continue to review full report at Codecov.
|
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.
Initial comments, will take a second look.
ambry-api/src/main/java/com.github.ambry/notification/NotificationSystem.java
Outdated
Show resolved
Hide resolved
ambry-api/src/main/java/com.github.ambry/notification/NotificationSystem.java
Outdated
Show resolved
Hide resolved
ambry-api/src/main/java/com.github.ambry/notification/NotificationSystem.java
Outdated
Show resolved
Hide resolved
* @param info The {@link MessageInfo} that carries some basic information about this operation. | ||
* @return the lifeVersion of the undeleted blob. | ||
*/ | ||
short undelete(MessageInfo info) throws StoreException; |
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.
Any reason why we don't pass in MessageWriteSet
? Is this because we undelete only one blob at a time?
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.
MessgeWriteSet includes a serialized undelete record, which will be persisted on disk in the log. The problem is that this serialized record requires a lifeVersion, but we don't know the lifeVersion before we query the index. So here I pass a MessageInfo instead of MessageWriteSet. You can see in the BlobStore.undelete method, I create a MessageWriteSet in the end, with a lifeVersion.
ambry-commons/src/main/java/com.github.ambry.commons/LoggingNotificationSystem.java
Show resolved
Hide resolved
StoreKey id = info.getStoreKey(); | ||
Offset indexEndOffsetBeforeCheck = index.getCurrentEndOffset(); | ||
List<IndexValue> values = index.findAllIndexValuesForKey(id, null); | ||
index.validateSanityForUndelete(id, values, IndexValue.LIFE_VERSION_FROM_FRONTEND); |
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.
Note that, other methods (put, ttlUpdate, delete) in BlobStore are also invoked in Replication. When you implement undelete replication part, the LIFE_VERSION_FROM_FRONTEND
check here probably needs to change.
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 actually have to change ttlupdate and delete later to make them aware of undelete in replication.
60e973c
to
a3f054e
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.
LGTM, one minor comment.
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.
LGTM.
One minor comment.
a3f054e
to
39ad87d
Compare
39ad87d
to
363a72d
Compare
Adding undelete method to blobstore and other Store implementation.
This PR will have no change w.r.t AmbryRequests.