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

HelixAccountService: limit the number of versions in the list #1252

Merged
merged 7 commits into from
Sep 10, 2019
Merged

HelixAccountService: limit the number of versions in the list #1252

merged 7 commits into from
Sep 10, 2019

Conversation

justinlin-linkedin
Copy link
Collaborator

Enforcing a upper bound on how many versions of account metadata to save in the ambry-server.

@codecov-io
Copy link

codecov-io commented Sep 4, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@9479711). Click here to learn what that means.
The diff coverage is 71.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1252   +/-   ##
=========================================
  Coverage          ?   72.28%           
  Complexity        ?     6123           
=========================================
  Files             ?      440           
  Lines             ?    35239           
  Branches          ?     4481           
=========================================
  Hits              ?    25472           
  Misses            ?     8603           
  Partials          ?     1164
Impacted Files Coverage Δ Complexity Δ
.../com/github/ambry/account/HelixAccountService.java 88.09% <100%> (ø) 43 <0> (?)
...github.ambry/config/HelixAccountServiceConfig.java 88.23% <100%> (ø) 2 <0> (?)
...ain/java/com/github/ambry/account/RouterStore.java 77.33% <69.64%> (ø) 10 <2> (?)

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 9479711...d84dd6e. Read the comment docs.

jsjtzyy
jsjtzyy reviewed Sep 7, 2019
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.

Looks good to me after the comments are addressed.

ambry-account/src/main/java/com/github/ambry/account/RouterStore.java
blobIDAndVersionsJson.stream()
.forEach(accountBlobIDInJson -> blobIDAndVersions.add(BlobIDAndVersion.fromJson(accountBlobIDInJson)));
Collections.sort(blobIDAndVersions, Comparator.comparing(BlobIDAndVersion::getVersion));
BlobIDAndVersion blobIDAndVersion = blobIDAndVersions.get(blobIDAndVersions.size() - 1);
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 this piece of code has same logic (correct me if I am wrong) with line106-109. If yes, can we make them consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the difference is that one need to sort the list and the other only has to get the latest blobIDAndVersion. I figure it's too small to create a function for.

cgtz
cgtz suggested changes Sep 9, 2019
View reviewed changes
ambry-account/src/main/java/com/github/ambry/account/RouterStore.java Outdated
iter.remove();
logger.info("Removing blob " + blobIDAndVersion.getBlobID() + " at version " + blobIDAndVersion.getVersion());
try {
router.get().deleteBlob(blobIDAndVersion.getBlobID(), SERVICE_ID).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any edge cases where we would not want to delete the oldest blob from within the update logic.
The main case to consider is probably where the update to zookeeper does not succeed. In this case we would have deleted a blob in ambry that could still exist in the current znode.

However, this may not be a problem if totalNumberOfVersionsToKeep is sufficiently large that the chance of someone else still interested in reading that blob is very small.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

delete the blobs after successfully updates zookeeper node.

cgtz
cgtz approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@cgtz cgtz left a comment

Choose a reason for hiding this comment

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

LGTM, just one small comment left

@cgtz cgtz merged commit dbecfae into linkedin:master Sep 10, 2019
@justinlin-linkedin justinlin-linkedin deleted the limitlistsize branch September 10, 2019 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsjtzyy jsjtzyy jsjtzyy left review comments

@cgtz cgtz cgtz approved these changes

@lightningrob lightningrob Awaiting requested review from lightningrob

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 @cgtz @jsjtzyy