Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add UndeleteMessageFormatInputStream #1363

Merged
merged 5 commits into from
Jan 27, 2020
Merged

Add UndeleteMessageFormatInputStream #1363

merged 5 commits into from
Jan 27, 2020

Conversation

justinlin-linkedin
Copy link
Collaborator

Adding UndeleteMessageFormatInputStream

@codecov-io
Copy link

codecov-io commented Jan 22, 2020 •
edited
Loading

Codecov Report

Merging #1363 into master will increase coverage by 0.03%.
The diff coverage is 84.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1363      +/-   ##
===========================================
+ Coverage     71.97%     72%   +0.03%     
- Complexity     6675    6676       +1     
===========================================
  Files           485     486       +1     
  Lines         38032   38060      +28     
  Branches       4820    4822       +2     
===========================================
+ Hits          27374   27407      +33     
+ Misses         9357    9349       -8     
- Partials       1301    1304       +3
Impacted Files Coverage Δ Complexity Δ
...essageformat/UndeleteMessageFormatInputStream.java 100% <100%> (ø) 1 <1> (?)
.../main/java/com.github.ambry/store/MessageInfo.java 89.41% <100%> (+0.25%) 25 <1> (+1) ⬆️
...ithub.ambry.messageformat/BlobStoreHardDelete.java 86.74% <75%> (-1.64%) 7 <0> (ø)
....github.ambry.messageformat/BlobStoreRecovery.java 76.34% <82.35%> (-2.97%) 7 <0> (ø)
...rc/main/java/com.github.ambry.store/BlobStore.java 89.26% <0%> (-0.9%) 101% <0%> (-3%)
...c/main/java/com.github.ambry.network/Selector.java 78.21% <0%> (-0.79%) 77% <0%> (-1%)
...va/com.github.ambry.replication/ReplicaThread.java 86.98% <0%> (-0.56%) 94% <0%> (ø)
...src/main/java/com.github.ambry.commons/BlobId.java 93.05% <0%> (-0.35%) 73% <0%> (-1%)
...ls/src/main/java/com.github.ambry.utils/Utils.java 77.24% <0%> (-0.33%) 116% <0%> (-1%)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 057f5a2...10d9507. Read the comment docs.

zzmao
zzmao approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@zzmao zzmao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jsjtzyy
jsjtzyy reviewed Jan 24, 2020
View reviewed changes
Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments, mostly looks good to me.

...ageformat/src/main/java/com.github.ambry.messageformat/UndeleteMessageFormatInputStream.java Outdated
@@ -0,0 +1,50 @@
/**
* Copyright 2016 LinkedIn Corp. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: 2020

...ageformat/src/main/java/com.github.ambry.messageformat/UndeleteMessageFormatInputStream.java Outdated

/**
* Represents a message that consists of the undelete record.
* This format is used to delete a blob
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: to undelete a blob

...ageformat/src/main/java/com.github.ambry.messageformat/UndeleteMessageFormatInputStream.java Outdated
int headerSize = MessageFormatRecord.getHeaderSizeForVersion(MessageFormatRecord.headerVersionToUse);
int recordSize = MessageFormatRecord.Update_Format_V3.getRecordSize(SubRecord.Type.UNDELETE);
buffer = ByteBuffer.allocate(headerSize + key.sizeInBytes() + recordSize);
MessageFormatRecord.MessageHeader_Format_V2.serializeHeader(buffer, recordSize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you decide to go with MessageHeader_Format_V2, which means lifeVersion is not tracked in message header. Is this intended?

ambry-api/src/main/java/com.github.ambry/store/MessageInfo.java
* @param accountId accountId of the blob
* @param containerId containerId of the blob
* @param operationTimeMs operation time in ms
* @param lifeVersion update version of update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add more comment for update operation (i.e DELETE/TTLUpdate/UNDELETE)

ambry-messageformat/src/main/java/com.github.ambry.messageformat/BlobStoreHardDelete.java
Comment on lines +83 to +86
short lifeVersion = 0;
if (headerFormat.hasLifeVersion()) {
lifeVersion = headerFormat.getLifeVersion();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't adopt MessageHeader_Format_V3, do we still need to check this? I am considering if lifeVersion is not put in header, we probably need to remove hasLifeVersion method from V1/V2 as well.

Copy link
Contributor

@jsjtzyy jsjtzyy Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, ignore this, I feel like we do need the lifeVersion in message header

...messageformat/src/test/java/com.github.ambry.messageformat/MessageFormatInputStreamTest.java Outdated
*/
private static void checkUndeleteMessage(InputStream stream, Long expectedRecordSize, StoreKey key, short accountId,
short containerId, long updateTimeMs) throws Exception {
checkHeaderAndStoreKeyForUpdate(stream, expectedRecordSize, key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we decide to use V3 header in the future, please add lifeVersion check in checkHeaderAndStoreKeyForUpdate method.

@jsjtzyy jsjtzyy merged commit 76f8d72 into linkedin:master Jan 27, 2020
@justinlin-linkedin justinlin-linkedin deleted the undeleterecord branch January 27, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zzmao zzmao zzmao approved these changes

@jsjtzyy jsjtzyy jsjtzyy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinlin-linkedin @codecov-io @jsjtzyy @zzmao