feat(cache): add hazelcast distributed cache option #6645

RyanHolstien · 2022-12-05T17:07:04Z

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

github-actions · 2022-12-05T17:53:57Z

Unit Test Results (build & test)

638 tests +17 634 ✔️ +17 16m 0s ⏱️ -1s
165 suites +  8     4 💤 ±  0
165 files +  8     0 ❌ ±  0

Results for commit 988a71b. ± Comparison against base commit fdcb731.

This pull request removes 4 and adds 21 tests. Note that renamed tests count towards both.

com.datahub.authorization.ranger.RangerAuthorizerTest ‑ testAuthorizationAllow
com.datahub.authorization.ranger.RangerAuthorizerTest ‑ testAuthorizationDeny
com.datahub.authorizer.plugin.ranger.TestDataHubRangerAuthPlugin ‑ testLookupResource
com.datahub.authorizer.plugin.ranger.TestDataHubRangerAuthPlugin ‑ testValidateConfig

com.datahub.plugins.auth.TestConfig ‑ testConfig
com.datahub.plugins.auth.TestConfigProvider ‑ testConfigurationLoading
com.datahub.plugins.auth.TestConfigValidationUtils ‑ testListShouldNotBeEmpty
com.datahub.plugins.auth.TestConfigValidationUtils ‑ testListShouldNotHaveDuplicate
com.datahub.plugins.auth.TestConfigValidationUtils ‑ testMapShouldNotBeEmpty
com.datahub.plugins.auth.TestConfigValidationUtils ‑ testWhiteSpacesValidation
com.datahub.plugins.auth.TestIsolatedClassLoader ‑ testAuthenticatorPlugin
com.datahub.plugins.auth.TestIsolatedClassLoader ‑ testAuthorizerPlugin
com.datahub.plugins.auth.TestIsolatedClassLoader ‑ testDuplicatePluginName
com.datahub.plugins.auth.TestIsolatedClassLoader ‑ testIncorrectImplementation
…

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-12-06T22:25:00Z

metadata-io/src/main/java/com/linkedin/metadata/search/cache/CachedEntityLineageResult.java

 import lombok.Data;


 @Data
 public class CachedEntityLineageResult {
-  private final EntityLineageResult entityLineageResult;
+  private final String entityLineageResult;


we are serializing this result? interesting

Yeah, this is essentially a DTO wrapper around the result to be able to include the timestamp (and anything else we want to add in later) onto the cached value so any RecordTemplate used within it has to be serialized

jjoyce0510 · 2022-12-06T22:26:04Z

metadata-service/factories/src/main/java/com/linkedin/gms/factory/common/CacheConfig.java

+  private int cacheMaxSize;
+
+  @Value("${searchService.cache.hazelcast.serviceName:hazelcast-service}")
+  private String hazelcastServiceName;


why do we have hazelcast in the name of variables here?
isn't the idea that we can swap in differnt impls for the caffeine cache?
does this seem overly-specific?

i see - so we are only accounting for 2 cases: caffeine and hazel. i think i get it

Spring Cache Manager is intended for very very simple use cases when switching out cache implementations. Unfortunately our use cases are complex enough where it requires using the provider level interfaces so we would need to reimplement any other implementations we support. This is unchanged from how it previously was, but just exposes the implementation name in the config. It's possible that other providers would also require a headless K8s service for their distributed cache implementation, but unlikely (if we even do implement another underlying supported cache) so I think it's okay to be specific here.

jjoyce0510 · 2022-12-06T22:28:50Z

metadata-service/factories/src/main/resources/application.yml

@@ -82,6 +82,10 @@ graphService:
 searchService:
  resultBatchSize: ${SEARCH_SERVICE_BATCH_SIZE:100}
  enableCache: ${SEARCH_SERVICE_ENABLE_CACHE:false}
+  cacheImplementation: ${SEARCH_SERVICE_CACHE_IMPLEMENTATION:caffeine}


minor minor: in terms of config layout, what if we did this

cache: type: "hazelcase" config: hazelcast: serviceName: .... same

this may read a bit easier. thoughts?

Agreed, I had it at the upper level to be in line with the enableCache variable, but I think it's fine to start a new pattern here.

jjoyce0510 · 2022-12-06T22:29:24Z

metadata-io/src/main/java/com/linkedin/metadata/search/cache/CacheableSearcher.java

+          Timer.Context cacheAccess = MetricUtils.timer(this.getClass(), "getBatch_cache_access").time();
+          K cacheKey = cacheKeyGenerator.apply(batch);
+          String json = cache.get(cacheKey, String.class);
+          result = json != null ? toRecordTemplate(SearchResult.class, json) : null;


Hazelcast requires that the cached object is a string? Or serializable? Cannot record template be serialized by itself?

Requires that it be serializable, RecordTemplate does not implement Serializable and any RecordTemplate being used in the key or value was throwing errors. Looked into trying to inject a custom deserializer into the Hazelcast deserialization config, but this was much easier.

jjoyce0510 · 2022-12-06T22:29:46Z

metadata-service/factories/src/test/java/com/linkedin/gms/factory/search/CacheTest.java

@@ -0,0 +1,64 @@
+package com.linkedin.gms.factory.search;
+
+import com.hazelcast.config.Config;


jjoyce0510 · 2023-01-10T16:13:09Z

metadata-io/src/main/java/com/linkedin/metadata/search/cache/CachedEntityLineageResult.java

-public class CachedEntityLineageResult {
-  private final EntityLineageResult entityLineageResult;
+public class CachedEntityLineageResult implements Serializable {
+  private final byte[] entityLineageResult;


Minor: Ideally this detail would be hidden to consumer of the CachedEntityLineageResult. We should auto deserialize it on "getEntityLineageResult"

jjoyce0510 · 2023-01-10T19:24:24Z

metadata-io/src/main/java/com/linkedin/metadata/search/cache/CachedEntityLineageResult.java


 @Data
 public class CachedEntityLineageResult implements Serializable {
  private final byte[] entityLineageResult;
  private final long timestamp;
+
+  public CachedEntityLineageResult(EntityLineageResult lineageResult, long timestamp) {
+    this.entityLineageResult = gzipCompress(toJsonString(lineageResult));


nice encapsulation!

definitely better than having external consumers needing to know about this internal detail

jjoyce0510

LGTM if it looks good to others.

…6645) Co-authored-by: Aseem Bansal <[email protected]>

RyanHolstien added 2 commits December 5, 2022 10:24

feat(cache): add hazelcast implementation

1fab4e7

squashed changes

8b8bb23

RyanHolstien requested a review from david-leifker December 5, 2022 17:07

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Dec 5, 2022

jjoyce0510 reviewed Dec 6, 2022

View reviewed changes

david-leifker approved these changes Dec 6, 2022

View reviewed changes

RyanHolstien added 3 commits December 7, 2022 15:52

fix(hazelcast): fix lineage caching

d082bb0

use string cachekey in both spots

988a71b

gzip cache entry and make serializable

dd42b7c

aditya-radhakrishnan added platform PR-s that make changes to core parts of the platform and removed product PR or Issue related to the DataHub UI/UX labels Dec 19, 2022

linting

ddf9f0c

jjoyce0510 reviewed Jan 10, 2023

View reviewed changes

review comments and reduce test workload

c06ea1e

jjoyce0510 reviewed Jan 10, 2023

View reviewed changes

jjoyce0510 approved these changes Jan 10, 2023

View reviewed changes

RyanHolstien and others added 2 commits January 12, 2023 11:30

prevent resource leaks

f5bdfe5

Merge branch 'master' into feat/distCache

e2bdcde

jjoyce0510 merged commit 8323128 into datahub-project:master Jan 19, 2023

RyanHolstien deleted the feat/distCache branch January 19, 2023 17:06

ericyomi pushed a commit to ericyomi/datahub that referenced this pull request Feb 8, 2023

feat(cache): add hazelcast distributed cache option (datahub-project#…

81db349

…6645) Co-authored-by: Aseem Bansal <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cache): add hazelcast distributed cache option #6645

feat(cache): add hazelcast distributed cache option #6645

RyanHolstien commented Dec 5, 2022

github-actions bot commented Dec 5, 2022 •

edited

Loading

jjoyce0510 Dec 6, 2022

RyanHolstien Dec 6, 2022

jjoyce0510 Dec 6, 2022

jjoyce0510 Dec 6, 2022

RyanHolstien Dec 6, 2022

jjoyce0510 Dec 6, 2022

RyanHolstien Dec 6, 2022

jjoyce0510 Dec 6, 2022

RyanHolstien Dec 6, 2022

jjoyce0510 Dec 6, 2022

jjoyce0510 Jan 10, 2023

jjoyce0510 Jan 10, 2023

jjoyce0510 Jan 10, 2023

jjoyce0510 left a comment

		@@ -0,0 +1,64 @@
		package com.linkedin.gms.factory.search;

		import com.hazelcast.config.Config;

feat(cache): add hazelcast distributed cache option #6645

feat(cache): add hazelcast distributed cache option #6645

Conversation

RyanHolstien commented Dec 5, 2022

Checklist

github-actions bot commented Dec 5, 2022 • edited Loading

Unit Test Results (build & test)

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

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

jjoyce0510 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2022 •

edited

Loading