[dvc][doc] Create MVP for DaVinciRecordTransformer #1087

kvargha · 2024-07-30T16:55:03Z

Summary

Added storeRecordsInDaVinci boolean to the constructor to detect if a user wants to store their transformed records in DaVinci or another storage of their choice.
Created DaVinciRecordTransformerConfig to be used by DaVinciConfig, so we have access to Output Value Class and Schema without needing to create a dummy record transformer outside of StoreIngestionTask.
Added a recovery method to bootstrap from VT or from local state on startup. If blob transfer is enabled, an error will be thrown since it's not currently supported with the record transformer.
Created AbstractStorageIterator and a RocksDB implementation to be used by the recovery method.
Added logic to detect if transformer logic has changed on startup, utilizing the hash of the class bytecode. If transformer logic has changed, that means local state is stale and we need to bootstrap directly from the VT.
Added logic to create a DVRT specific Avro deserializer.
Split put into transform and processPut, so that when we're trying to bootstrap from local state, we don't try to transform the data again as it already has been transformed. Also created a wrapper method transformAndProcessPut when both need to be called in StoreIngestionTask.
Created DaVinciRecordTransformerResult, to handle cases where the user doesn't transform the value or wants to skip it.
Added delete logic in StoreIngestionTask.
Throw an exception when a user tries to use Ingestion Isolation with DVRT. This can cause issues if a user's transformer is stateful, and it will cause the storage engine to be deleted every time the client comes online.
Updated record transformer doc with new logic.
Made Venice repo a link in the workspace setup doc.

How was this PR tested?

Added more unit and integration tests.

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

FelixGV · 2024-07-30T19:27:01Z

The thing about doing a hash of the bytecode sounds pretty interesting, but I wonder how reliable it is... I suspect that it probably has extremely low chances of false negative (the code changed but we fail to detect it) but that it might have a somewhat large probability of false positives (the code did not change but we think it did). The latter could be due to differences in JDK versions or flags used for compilation... Maybe it's not a big deal, but IDK...

Also, coming back to false negatives, those may still exist if the scope of the checksumming is not large enough, e.g. the class we checksum did not change but it calls into another class which did change.

Overall, I am not sure how I feel about this mechanism, and I think we may want to have a config to opt out of this check. Possibly even: this should be opt in to begin with (i.e. off by default) until we can gain a better understanding of its limitations.

kvargha · 2024-07-30T21:18:30Z

The thing about doing a hash of the bytecode sounds pretty interesting, but I wonder how reliable it is... I suspect that it probably has extremely low chances of false negative (the code changed but we fail to detect it) but that it might have a somewhat large probability of false positives (the code did not change but we think it did). The latter could be due to differences in JDK versions or flags used for compilation... Maybe it's not a big deal, but IDK...

Also, coming back to false negatives, those may still exist if the scope of the checksumming is not large enough, e.g. the class we checksum did not change but it calls into another class which did change.

Overall, I am not sure how I feel about this mechanism, and I think we may want to have a config to opt out of this check. Possibly even: this should be opt in to begin with (i.e. off by default) until we can gain a better understanding of its limitations.

From my testing, the hash would change under these circumstances:

Change in JDK version.
Any changes made within the user's implementation (I found this to be very reliable).

Circumstances where it didn't change:

Change in version numbers of imported packages that's used by the user's implementation.
Code changes in functions imported.
Changes in the abstract class.

I think having it off by default and having the user enable it makes sense.

We could have a global variable that's initially set to false, and add another constructor where a user can pass in a boolean of whether or not to enable it. We could call the variable detectChangesInLogic. If it's set to false, then always bootstrap from VT. Thoughts @ZacAttack?

Dvrt mvp

…nstructor

…former

clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java

clients/da-vinci-client/src/main/java/com/linkedin/davinci/client/DaVinciRecordTransformer.java

clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java

clients/da-vinci-client/src/main/java/com/linkedin/davinci/StoreBackend.java

clients/da-vinci-client/src/main/java/com/linkedin/davinci/client/AvroGenericDaVinciClient.java

* Create MVP for record transformer * Fix docs and imports * Don't make serializers and deserializers global * Dont check for boolean val * Delete classHash if it exists * Assert file deletion is true * Improve code coverage * Add tests for blocking record transformer * Created AvroGenericDaVinciClient for record transformer test * Make sure getRecordTransformer is valid * Make sure getRecordTransformer isn't null * Reorganize testRecordTransformerClient and fix key schema * Fix TestStringRecordTransformer to work with Avro objects and update doc * Refactor onRecovery and add javadoc for DaVinciRecordTransformer's constructor * Reset offset if we need to bootstrap from VT * Delete classHash after running tests * Throw an error if a user tries to use blob transfer with record transformer * Make previous public methods private, remove subscribe call inside onRecovery * Correctly pass DVRT functional interface to initBackend, add todo to store classHash in rocksDB * Fix spotbugs * Init DVRT inside SIT, and move DVRT recovery to SIT * Modify checkout action * Undo * Fix compilation * Cache deserializer/serializer * Create utility class for record transformer * Create AbstractStorageIterator and a RocksDB implementation * Delete classHash file. Compare value classes and don't override value class in AvroGenericDaVinciClient * Remove compareCacheConfig mock * Wrap access modifier with doPrivileged * Fix spotless error * Remove unused variables * Added a ToDo to make chunking with record transformer lazy, and make the return type of delete void. Proceed with deletion based off of storeRecordsInDaVinci * Created a config for record transformer that's passed into the DaVinciConfig to remove the need of instantiating a dummy record transformer, updated docs, and simplified function names * Add message envelope for DVRT * Fix spotless issue * Fix test * update docs

...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java

...-vinci-client/src/main/java/com/linkedin/davinci/client/DaVinciRecordTransformerUtility.java

clients/da-vinci-client/src/main/java/com/linkedin/davinci/client/DaVinciRecordTransformer.java

kvargha

ToDo: Throw error if Ingestion Isolation is turned on with DVRT.

...-vinci-client/src/main/java/com/linkedin/davinci/client/DaVinciRecordTransformerUtility.java

ZacAttack · 2024-10-28T20:41:41Z

...ommon/src/integrationTest/java/com/linkedin/venice/endToEnd/TestStringRecordTransformer.java

+    return new DaVinciRecordTransformerResult<>(DaVinciRecordTransformerResult.Result.TRANSFORMED, transformedValue);
+  }
+
+  public void processPut(Lazy<Integer> key, Lazy<String> value) {


random: Shouldn't this @OverRide?

ZacAttack · 2024-10-28T20:57:51Z

...ice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/DaVinciClientTest.java

+      }
+
+      // Mimic restart
+      clientWithRecordTransformer.close();


Just to make sure I understand the implication of this test, does this run through on disk state on restart? Or.... does it actually reingest?

The former. I used the debugger to verify that it's actually reading from disk and not re-ingesting.

ZacAttack · 2024-10-28T21:02:37Z

...ice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/DaVinciClientTest.java

+  }
+
+  @Test(timeOut = TEST_TIMEOUT, dataProvider = "dv-client-config-provider", dataProviderClass = DataProviderUtils.class)
+  public void testRecordsInDaVinciDisabledRecordTransformer(DaVinciConfig clientConfig) throws Exception {


I don't understand this test. What exactly is this case??

This tests the case where a user passes false into the constructor for storeRecordsInDaVinci.

That means nothing should be stored in DaVinci.

ZacAttack · 2024-10-28T21:15:52Z

clients/da-vinci-client/src/main/java/com/linkedin/davinci/client/AvroGenericDaVinciClient.java

@@ -657,7 +667,7 @@ protected GenericRecordChunkingAdapter getGenericRecordChunkingAdapter() {
    return GenericRecordChunkingAdapter.INSTANCE;
  }

-  private D2ServiceDiscoveryResponse discoverService() {
+  D2ServiceDiscoveryResponse discoverService() {


protected? Or /* package private */ ?

ZacAttack · 2024-10-28T21:18:34Z

clients/da-vinci-client/src/main/java/com/linkedin/davinci/client/AvroGenericDaVinciClient.java

+
+      if (clientConfig.isSpecificClient()) {
+        if (daVinciConfig.isRecordTransformerEnabled()) {
+          if (recordTransformerConfig.getOutputValueClass() != clientConfig.getSpecificValueClass()) {


Let's do a test case through these, I think looking back at the integration test cases the transformed type is always the same.

kvargha added 13 commits July 29, 2024 13:27

Create MVP for record transformer

8dc58f0

Fix docs and imports

88b62fe

Don't make serializers and deserializers global

fc856b7

Dont check for boolean val

03f0453

Delete classHash if it exists

45ba862

Assert file deletion is true

e8fa547

Improve code coverage

b3f3e0a

Add tests for blocking record transformer

f3cacff

Created AvroGenericDaVinciClient for record transformer test

d2532b3

Make sure getRecordTransformer is valid

4d3ee2b

Make sure getRecordTransformer isn't null

189127c

Merge branch 'main' into dvrt-mvp

9995758

Reorganize testRecordTransformerClient and fix key schema

2c421a6

kvargha and others added 7 commits July 31, 2024 11:12

Fix TestStringRecordTransformer to work with Avro objects and update doc

68d2be8

Merge pull request #1 from kvargha/dvrt-mvp

9ffbf41

Dvrt mvp

Refactor onRecovery and add javadoc for DaVinciRecordTransformer's co…

fc0b188

…nstructor

Reset offset if we need to bootstrap from VT

05c22b4

Merge branch 'main' into dvrt-mvp

1ef7c20

Delete classHash after running tests

6b56ff8

Throw an error if a user tries to use blob transfer with record trans…

2cbd14f

…former

ZacAttack reviewed Aug 9, 2024

View reviewed changes

clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java Outdated Show resolved Hide resolved

ZacAttack reviewed Aug 9, 2024

View reviewed changes

clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java Outdated Show resolved Hide resolved