Implement helixAccountService with version history and blobs #1234

justinlin-linkedin · 2019-08-12T19:03:40Z

This is the first step of implementation for this design https://docs.google.com/document/d/1Lr30Uph1KL0AMHznTGEdC_JG3W6_d_NKNTa9FbBTTjI/edit

In this implementation, HelixAccountService has two separated implementation of AccountMetadataStore, which fetches account metadata and update it.

Old approach is still the default implementation and new approach would be enabled after.

The next step is to

implement a function to copy the account metadata from the old path to ambry server and create that blob id list.
limit the size of list by removing the blobs when the size of list exceeds the count in the config.

Notice that this implementation might end up with some blob leak.

codecov-io · 2019-08-12T19:23:03Z

Codecov Report

Merging #1234 into master will decrease coverage by 3.07%.
The diff coverage is 81.79%.

@@             Coverage Diff              @@
##             master    #1234      +/-   ##
============================================
- Coverage     88.92%   85.84%   -3.08%     
- Complexity       60       76      +16     
============================================
  Files             6        9       +3     
  Lines           352      544     +192     
  Branches         37       56      +19     
============================================
+ Hits            313      467     +154     
- Misses           29       58      +29     
- Partials         10       19       +9

Impacted Files	Coverage Δ	Complexity Δ
...com/github/ambry/account/AccountMetadataStore.java	`100% <100%> (ø)`	`5 <5> (?)`
...om/github/ambry/account/AccountServiceMetrics.java	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
...ain/java/com/github/ambry/account/RouterStore.java	`77.08% <77.08%> (ø)`	`11 <11> (?)`
.../com/github/ambry/account/HelixAccountService.java	`87.05% <80.61%> (-0.94%)`	`33 <7> (-5)`
.../com/github/ambry/account/LegacyMetadataStore.java	`86.79% <86.79%> (ø)`	`5 <5> (?)`

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 e5d6e7e...ce88e0b. Read the comment docs.

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

lightningrob

Left a few initial comments. I will re-review after HelixAccountService is refactored.

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

ambry-account/src/test/java/com/github/ambry/account/HelixAccountServiceTest.java

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java

ambry-account/src/main/java/com/github/ambry/account/RouterStore.java

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

ambry-account/src/main/java/com/github/ambry/account/RouterStore.java

justinlin-linkedin

updated

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

Implement helixAccountService with version history Add tests Before changing the router store Add more test

lightningrob

LGTM

jsjtzyy

Some comments. Need to take a look at tests.

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java

ambry-account/src/main/java/com/github/ambry/account/LegacyMetadataStore.java

ambry-api/src/main/java/com.github.ambry/config/HelixAccountServiceConfig.java

jsjtzyy · 2019-08-15T22:21:44Z

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

    if (notifier != null) {
      notifier.subscribe(ACCOUNT_METADATA_CHANGE_TOPIC, changeTopicListener);
    } else {
      logger.warn("Notifier is null. Account updates cannot be notified to other entities. Local account cache may not "
          + "be in sync with remote account data.");
      accountServiceMetrics.nullNotifierCount.inc();
    }
+    if (config.useNewZNodePath) {
+      accountMetadataStore = new RouterStore(this.accountServiceMetrics, backup, helixStore, router);


I understand this is first PR to improve account metadata and I believe setupRouter will be invoked in future PR, however, here router might not be initialized, can we do some proactive check in RouterStore?
Eventually, after transition is done, we should explicitly call setupRouter in RestServer.java

good idea, updated

jsjtzyy · 2019-08-15T22:52:13Z

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java

+   * @param isCalledFromListener True is this function is invoked in the {@link TopicListener}.
+   */
+  private synchronized void fetchAndUpdateCache(boolean isCalledFromListener) {
+    Map<String, String> accountMap = accountMetadataStore.fetchAccountMetadata();


Although this is synchronized method, there might be race condition when initialFetchAndSchedule is called before accountMetadataStore is instantiated. My suggestion is, if possible, move notifier.subscribe() to the end of constructor.

Good Idea, I move the notifier.subscribe into the initialFetchAndSchedule function so there will be no race condition here.

jsjtzyy

Looks good after minor comments are addressed.

jsjtzyy · 2019-08-15T23:37:24Z

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java

+    ZNRecord znRecord = helixStore.get(znRecordPath, null, AccessOption.PERSISTENT);
+    logger.trace("Fetched ZNRecord from path={}, took time={} ms", znRecordPath, startTimeMs);
+    if (znRecord == null) {
+      logger.debug("The ZNRecord to read does not exist on path={}", znRecordPath);


minor: I think here can be warn

but it's not a warning message. ZNRecord can be null here. It's one of the legit state.

If not warn, I would suggest info because such info is supposed to be printed out (debug level is turned off by default). I feel like ZNRecord should exist in almost all cases, if not, it's actually a warning that either the ZNode is deleted or znRecordPath is incorrect.

jsjtzyy · 2019-08-15T23:41:11Z

ambry-account/src/main/java/com/github/ambry/account/RouterStore.java

+    }
+    List<String> accountBlobIDs = record.getListField(ACCOUNT_METADATA_BLOB_IDS_LIST_KEY);
+    if (accountBlobIDs == null || accountBlobIDs.size() == 0) {
+      logger.debug("ZNRecord={} to read on path={} does not have a simple list with key={}", record,


same here, maybe use warn?

jsjtzyy · 2019-08-16T00:50:53Z

ambry-account/src/main/java/com/github/ambry/account/RouterStore.java

+          throw new IllegalStateException(errorMessage, e);
+        } catch (Exception e) {
+          errorMessage =
+              String.format("Unexpected exception occurred when parsing the blob id list from {}", accountBlobIDs);


if you choose to String.format, can you change it to following style (basically replace {} with %s):

String.format("Unexpected exception occurred when parsing the blob id list from %s", accountBlobIDs);

Make change to all occurrences. This can pass intellij inspection.

figured that string concatenation would just do the work.

justinlin-linkedin requested review from cgtz and jsjtzyy August 12, 2019 19:03

lightningrob reviewed Aug 12, 2019

View reviewed changes

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java Outdated Show resolved Hide resolved

lightningrob requested changes Aug 12, 2019

View reviewed changes

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java Outdated Show resolved Hide resolved

ambry-account/src/test/java/com/github/ambry/account/HelixAccountServiceTest.java Outdated Show resolved Hide resolved

justinlin-linkedin requested a review from lightningrob August 12, 2019 23:00

lightningrob requested changes Aug 14, 2019

View reviewed changes

justinlin-linkedin commented Aug 15, 2019

View reviewed changes

justinlin-linkedin force-pushed the helixas branch from dc5ec93 to 5e4ddcb Compare August 15, 2019 17:59

lightningrob reviewed Aug 15, 2019

View reviewed changes

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java Outdated Show resolved Hide resolved

lightningrob reviewed Aug 15, 2019

View reviewed changes

ambry-account/src/main/java/com/github/ambry/account/AccountMetadataStore.java Show resolved Hide resolved

lightningrob reviewed Aug 15, 2019

View reviewed changes

ambry-account/src/main/java/com/github/ambry/account/HelixAccountService.java Outdated Show resolved Hide resolved

justinlin-linkedin force-pushed the helixas branch from 5e4ddcb to d0efbc4 Compare August 15, 2019 20:33

justinlin-linkedin added 9 commits August 15, 2019 14:05

Implement helixAccountService with version history and blobs

8223b43

Implement helixAccountService with version history Add tests Before changing the router store Add more test

nit

78342cf

Comments

c85f61e

Separate classes to different files

53b5794

nit

e51d9e8

Comments

90c8547

nit

ccfbea2

Comments

a19b5bf

Comments and reformat

bd5b99c

justinlin-linkedin force-pushed the helixas branch from dce6bd2 to bd5b99c Compare August 15, 2019 21:05

lightningrob approved these changes Aug 15, 2019

View reviewed changes

jsjtzyy reviewed Aug 15, 2019

View reviewed changes

Comments

7bd58ae

jsjtzyy approved these changes Aug 16, 2019

View reviewed changes

justinlin-linkedin added 2 commits August 15, 2019 20:17

Formatting

c899b40

comments

ce88e0b

jsjtzyy approved these changes Aug 16, 2019

View reviewed changes

jsjtzyy merged commit bbaf997 into linkedin:master Aug 16, 2019

justinlin-linkedin deleted the helixas branch August 16, 2019 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement helixAccountService with version history and blobs #1234

Implement helixAccountService with version history and blobs #1234

justinlin-linkedin commented Aug 12, 2019

codecov-io commented Aug 12, 2019 •

edited

Loading

lightningrob left a comment

justinlin-linkedin left a comment

lightningrob left a comment

jsjtzyy left a comment

jsjtzyy Aug 15, 2019

justinlin-linkedin Aug 15, 2019

jsjtzyy Aug 15, 2019

justinlin-linkedin Aug 15, 2019

jsjtzyy left a comment

jsjtzyy Aug 15, 2019

justinlin-linkedin Aug 16, 2019

jsjtzyy Aug 16, 2019

jsjtzyy Aug 15, 2019

jsjtzyy Aug 16, 2019

justinlin-linkedin Aug 16, 2019

Implement helixAccountService with version history and blobs #1234

Implement helixAccountService with version history and blobs #1234

Conversation

justinlin-linkedin commented Aug 12, 2019

codecov-io commented Aug 12, 2019 • edited Loading

Codecov Report

lightningrob left a comment

Choose a reason for hiding this comment

justinlin-linkedin left a comment

Choose a reason for hiding this comment

lightningrob left a comment

Choose a reason for hiding this comment

jsjtzyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 12, 2019 •

edited

Loading