-
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
Improve the implementation of backup in HelixAccountService #1246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1246 +/- ##
============================================
+ Coverage 72.21% 72.27% +0.06%
- Complexity 6063 6109 +46
============================================
Files 439 440 +1
Lines 35002 35145 +143
Branches 4446 4465 +19
============================================
+ Hits 25276 25402 +126
- Misses 8571 8584 +13
- Partials 1155 1159 +4
Continue to review full report at Codecov.
|
ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
backupFiles.put(version, currentBackup); | ||
} else { | ||
// remove the current file | ||
currentBackup.tryRemove(); |
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.
Huh? Isn't the idea to remove the oldest saved file to make room for the new one?
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.
yes, but here the currentBackup's version is less than the smallest version number in the map. So current backup file should be removed.
* @param afterTimeInSecond The unix epoch time which the latest backup's modifiedTime must be greater than. | ||
* @return The account map from the latest backup file. | ||
*/ | ||
Map<String, String> getLatestState(long afterTimeInSecond) { |
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.
Rename variable, eg. latestTimeAllowed.
* @param afterTimeInSecond The unix epoch time which the latest backup's modifiedTime must be greater than. | ||
* @return The account map from the latest backup file. | ||
*/ | ||
Map<String, String> getLatestState(long afterTimeInSecond) { |
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.
Instead of returning null, how about Optional<Map<>>?
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.
Not sure if Optional is better than just null, since the caller function just want to check if there is a result or not.
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/LocalBackup.java
Outdated
Show resolved
Hide resolved
0eb953f
to
8bdb461
Compare
* Delete file identified by the given {@link Path}. | ||
* @param toDelete The path of file to be deleted. | ||
*/ | ||
private void deleteFile(Path toDelete) { |
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.
You can call this tryDeleteFile too.
8bdb461
to
adde0cd
Compare
a991698
to
9c0af29
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.
a few comments, will review when it is unblocked
ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
ambry-account/src/main/java/com/github/ambry/account/BackupFileManager.java
Outdated
Show resolved
Hide resolved
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.
Mostly looks good. Will approve after a few comments are addressed.
Change the semantics of backup files in the HelixAccountService