[da-vinci][dvc] Dropping unassigned partitions #1332

kristyelee · 2024-11-20T23:33:41Z

Summary, imperative, start upper case, don't end with a period

The objective is to remove unassigned partitions.
Applying this issue to Da Vinci Client which has information over partitions in storage.

This PR is an extension of the PR #1196 [https://github.com//pull/1196] that checks which partitions should be kept [in StorageService] and applies the check in VeniceServer.

Resolves #650

How was this PR tested?

A corresponding test will be written.

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.

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

…iption

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

…iption. Enabled config flag over bootstrap subscription

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

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

…cally. Integration Test [In Writing]

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

…cally. Integration Test [In Writing]

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

ZacAttack · 2024-12-16T17:28:12Z

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

-      StoreBackend storeBackend = getStoreOrThrow(storeName);
-      storeBackend.subscribe(ComplementSet.newSet(partitions), Optional.of(version));
-    });
+    if (configLoader.getCombinedProperties().getBoolean(DA_VINCI_SUBSCRIBE_ON_DISK_PARTITIONS_AUTOMATICALLY, true)) {


This looks like a good start, but do we need to do clean up? @sixpluszero @kvargha ?

Maybe we add a method to the interface which allows users to clean up unsubscribed state?

Gaojie proposed that we keep the state on disk to play it on the safe side, as with a new version it will automatically get cleaned up.

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

…cally. Integration Test [In Writing]

kvargha · 2025-01-08T18:25:35Z

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

+      // Test automatic new version ingestion
+      for (int i = 0; i < 2; ++i) {
+        // Test per-version partitioning parameters
+        int partitionCount = i + 1;
+        String iString = String.valueOf(i);
+        cluster.useControllerClient(controllerClient -> {
+          ControllerResponse response = controllerClient.updateStore(
+              storeName1,
+              new UpdateStoreQueryParams().setPartitionerClass(ConstantVenicePartitioner.class.getName())
+                  .setPartitionCount(partitionCount)
+                  .setPartitionerParams(
+                      Collections.singletonMap(ConstantVenicePartitioner.CONSTANT_PARTITION, iString)));
+          assertFalse(response.isError(), response.getError());
+        });
+
+        Integer expectedValue = cluster.createVersion(storeName1, KEY_COUNT);
+        TestUtils.waitForNonDeterministicAssertion(TEST_TIMEOUT, TimeUnit.MILLISECONDS, () -> {
+          for (int k = 0; k < KEY_COUNT; ++k) {
+            Object readValue = client1.get(k).get();
+            assertEquals(readValue, expectedValue);
+          }
+        });
+      }
+    }


I don't think we need this for this test. We can focus on one version to determine whether or not we're not automatically subscribing to partitions already on disk.

kvargha · 2025-01-08T18:30:52Z

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

+      client1.subscribe(partitions);
+      client1.unsubscribeAll();
+
+      DaVinciBackend daVinciBackend = AvroGenericDaVinciClient.getBackend();
+      if (daVinciBackend != null) {
+        StoreBackend storeBackend = daVinciBackend.getStoreOrThrow(storeName1);
+        ComplementSet<Integer> subscription = storeBackend.getSubscription();
+        assertTrue(subscription.isEmpty());


What we can do here instead is we can stick with the default partition count of 3 instead of modifying it, and in the new client we only subscribe to 2 partitions and validate that subscription only has 2 in the ComplementSet.

…ally. Integration Test [In Writing]

kristyelee added 2 commits November 20, 2024 15:15

Initial commit

d282196

Merge branch 'linkedin:main' into kristy_lee/650_davinci

e7aca2e

kristyelee marked this pull request as draft November 20, 2024 23:33

kristyelee added 6 commits November 25, 2024 10:41

Test Commit

ec82b2c

Test Commit

5fa6390

Test unsubscription of partitions

6601afd

Rewrite unsubscription of partitions

5b18fe9

Merge branch 'linkedin:main' into kristy_lee/650_davinci

e5e6046

Modify Partition Removal

ef49d50

kristyelee commented Nov 26, 2024

View reviewed changes

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

kristyelee added 4 commits November 27, 2024 15:14

Partition adjustment in bootstrapping [DaVinciBackend]

0059955

Merge branch 'linkedin:main' into kristy_lee/650_davinci

b32d787

[dvc] Partition Difference Calculation and Update to Partition Subscr…

1fa3eba

…iption

Integration Test [In Writing]

321fb40

kvargha reviewed Dec 4, 2024

View reviewed changes

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

Partition Removal

52a557c

kristyelee force-pushed the kristy_lee/650_davinci branch from 2055b04 to 52a557c Compare December 5, 2024 01:08

kristyelee added 3 commits December 5, 2024 12:21

Integration Test [In Writing]

c5c9f16

Integration Test [In Writing]

b520ab8

Integration Test [In Writing]

262bbc6

kristyelee force-pushed the kristy_lee/650_davinci branch from b94b49a to 262bbc6 Compare December 9, 2024 18:32

kristyelee added 2 commits December 9, 2024 13:11

[dvc] Partition Difference Calculation and Update to Partition Subscr…

699c200

…iption. Enabled config flag over bootstrap subscription

[dvc] Partition Difference Calculation and Update to Partition Subscr…

002d55c

…iption. Enabled config flag over bootstrap subscription

kvargha reviewed Dec 10, 2024

View reviewed changes

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

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java Outdated Show resolved Hide resolved

kristyelee force-pushed the kristy_lee/650_davinci branch from a163e56 to 9dbffc9 Compare December 10, 2024 23:45

[dvc] Config flag enabled for subscribing on disk partitions automati…

fd9fa02

…cally. Integration Test [In Writing]

kristyelee force-pushed the kristy_lee/650_davinci branch from 9dbffc9 to fd9fa02 Compare December 11, 2024 00:59

[dvc] Config flag enabled for subscribing on disk partitions automati…

69d619a

…cally. Integration Test [In Writing]

kvargha reviewed Dec 13, 2024

View reviewed changes

...ice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/DaVinciClientTest.java Outdated Show resolved Hide resolved

[dvc] Config flag enabled for subscribing on disk partitions automati…

ac32242

…cally. Integration Test [In Writing]

[dvc] Config flag enabled for subscribing on disk partitions automati…

9554248

…cally. Integration Test [In Writing]

ZacAttack reviewed Dec 16, 2024

View reviewed changes

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

ZacAttack reviewed Dec 16, 2024

View reviewed changes

...ice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/DaVinciClientTest.java Outdated Show resolved Hide resolved

ZacAttack reviewed Dec 16, 2024

View reviewed changes

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

[dvc] Config flag enabled for subscribing on disk partitions automati…

7b20b28

…cally. Integration Test [In Writing]

kvargha reviewed Jan 8, 2025

View reviewed changes

[dvc] Config flag enabled for subscribing on disk partition automatic…

9bfe418

…ally. Integration Test [In Writing]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[da-vinci][dvc] Dropping unassigned partitions #1332

[da-vinci][dvc] Dropping unassigned partitions #1332

kristyelee commented Nov 20, 2024

ZacAttack Dec 16, 2024

ZacAttack Dec 16, 2024

kvargha Jan 8, 2025

kvargha Jan 8, 2025 •

edited

Loading

kvargha Jan 8, 2025

[da-vinci][dvc] Dropping unassigned partitions #1332

Are you sure you want to change the base?

[da-vinci][dvc] Dropping unassigned partitions #1332

Conversation

kristyelee commented Nov 20, 2024

Summary, imperative, start upper case, don't end with a period

How was this PR tested?

Does this PR introduce any user-facing changes?

ZacAttack Dec 16, 2024

Choose a reason for hiding this comment

ZacAttack Dec 16, 2024

Choose a reason for hiding this comment

kvargha Jan 8, 2025

Choose a reason for hiding this comment

kvargha Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

kvargha Jan 8, 2025

Choose a reason for hiding this comment

kvargha Jan 8, 2025 •

edited

Loading