From cc1d7ffb318020f4b39b0b8bcb01116def377eda Mon Sep 17 00:00:00 2001 From: = <18691669+kristyelee@users.noreply.github.com> Date: Thu, 12 Sep 2024 12:11:31 -0700 Subject: [PATCH 01/48] Initial commit --- .../davinci/store/rocksdb/RocksDBStoragePartition.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java index 0d1728b3bc..e224cc0988 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java @@ -817,6 +817,10 @@ private void deleteDirectory(String fullPath) { } } + /** + * Note: POI for removing folders for partitions not assigned, for given store_version. + */ + @Override public synchronized void drop() { close(); From c4b6682d2c0324ee0f5ecf75ee0fb4d59bd0fc22 Mon Sep 17 00:00:00 2001 From: = <18691669+kristyelee@users.noreply.github.com> Date: Mon, 16 Sep 2024 17:25:30 -0700 Subject: [PATCH 02/48] First test for removal of partition --- .../store/AbstractStorageEngineTest.java | 21 +++++++++++++++++++ .../rocksdb/RocksDBStorageEngineTest.java | 5 +++++ 2 files changed, 26 insertions(+) diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java index cbe8b0117b..a19a4831a9 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java @@ -126,6 +126,27 @@ public void testAddingAPartitionTwice() throws Exception { Assert.fail("Adding the same partition:" + partitionId + " again did not throw any exception as expected."); } + public void testRemovingPartition() throws Exception { + + init(); + + // first, add partition + doAddPartition(partitionId); + + if (!testStore.containsPartition(partitionId)) { + Assert.fail("Adding a new partition: " + partitionId + "failed!"); + } + + // remove existing partition + doRemovePartition(partitionId); + + Assert.assertEquals( + testStoreEngine.containsPartition(partitionId), + false, + "Failed to remove partition: " + partitionId + " from the store engine!"); + + } + public void testRemovingPartitionTwice() throws Exception { init(); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java index 1af4b08fee..f2028d3a14 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java @@ -183,6 +183,11 @@ public void testAddingAPartitionTwice() throws Exception { super.testAddingAPartitionTwice(); } + @Test + public void testRemovingPartition() throws Exception { + super.testRemovingPartition(); + } + @Test public void testRemovingPartitionTwice() throws Exception { super.testRemovingPartitionTwice(); From 8093581bfc2548a35b8674182ca65af867ab225c Mon Sep 17 00:00:00 2001 From: = <18691669+kristyelee@users.noreply.github.com> Date: Tue, 17 Sep 2024 17:10:38 -0700 Subject: [PATCH 03/48] Write function to remove unsubscribed (unassigned) partitions --- .../davinci/kafka/consumer/StoreIngestionTask.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java index d8e22e1ca1..4b4cf675c6 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java @@ -575,6 +575,19 @@ void resubscribeForAllPartitions() throws InterruptedException { } } + /** + * Removes partitions that are not subscribed / not assigned. + */ + + public synchronized void removeUnsubscribedPartitions() { + throwIfNotRunning(); + for (PartitionConsumptionState partitionConsumptionState: partitionConsumptionStateMap.values()) { + if (!partitionConsumptionState.isSubscribed()) { + partitionConsumptionStateMap.remove(partitionConsumptionState.getPartition(), partitionConsumptionState); + } + } + } + /** * Adds an asynchronous partition subscription request for the task. */ From 3eaeafbd440ead179acc57a93ab659ab54b19531 Mon Sep 17 00:00:00 2001 From: = <18691669+kristyelee@users.noreply.github.com> Date: Tue, 17 Sep 2024 18:09:59 -0700 Subject: [PATCH 04/48] Write test function to test removing unsubscribed (unassigned) partitions. --- .../kafka/consumer/StoreIngestionTaskTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java index a5b5969a80..9a78b34f06 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java @@ -2034,6 +2034,21 @@ public void testUnsubscribeConsumption(AAConfig aaConfig) throws Exception { }, aaConfig); } + @Test(dataProvider = "aaConfigProvider") + public void testRemoveUnsubscribedPartitions(AAConfig aaConfig) throws Exception { + localVeniceWriter.broadcastStartOfPush(new HashMap<>()); + localVeniceWriter.put(putKeyFoo, putValue, SCHEMA_ID); + + runTest(Utils.setOf(PARTITION_FOO), () -> { + verify(mockLogNotifier, timeout(TEST_TIMEOUT_MS)).started(topic, PARTITION_FOO); + // Start of push has already been consumed. Stop consumption. + storeIngestionTaskUnderTest.unSubscribePartition(fooTopicPartition); + // Unassigned partitions should be removed. + storeIngestionTaskUnderTest.removeUnsubscribedPartitions(); + verify(mockLogNotifier, timeout(TEST_TIMEOUT_MS)).stopped(anyString(), anyInt(), anyLong()); + }, aaConfig); + } + @Test(dataProvider = "aaConfigProvider") public void testKillConsumption(AAConfig aaConfig) throws Exception { final Thread writingThread = new Thread(() -> { From 0e24cec2ecc3ca0d2de2cd5c474ab53bbe1e370d Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 23 Sep 2024 15:32:26 -0700 Subject: [PATCH 05/48] Update StorageEngine intializer in VeniceServer --- .../venice/helix/SafeHelixDataAccessor.java | 2 +- .../linkedin/venice/server/VeniceServer.java | 54 +++++++++++++++---- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java b/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java index afff1b42de..f4eb45bc98 100644 --- a/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java +++ b/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java @@ -82,7 +82,7 @@ public boolean updateProperty(PropertyKey key, DataUpd return helixDataAccessor.updateProperty(key, updater, value); } - public T getProperty(PropertyKey key) { + public static T getProperty(PropertyKey key) { return helixDataAccessor.getProperty(key); } diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 206e5a2465..c8345a32d6 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -19,6 +19,7 @@ import com.linkedin.davinci.storage.StorageEngineRepository; import com.linkedin.davinci.storage.StorageMetadataService; import com.linkedin.davinci.storage.StorageService; +import com.linkedin.davinci.store.AbstractStorageEngine; import com.linkedin.venice.acl.DynamicAccessController; import com.linkedin.venice.acl.StaticAccessController; import com.linkedin.venice.blobtransfer.BlobTransferManager; @@ -34,6 +35,7 @@ import com.linkedin.venice.helix.HelixCustomizedViewOfflinePushRepository; import com.linkedin.venice.helix.HelixInstanceConfigRepository; import com.linkedin.venice.helix.HelixReadOnlyZKSharedSchemaRepository; +import com.linkedin.venice.helix.SafeHelixDataAccessor; import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.helix.ZkAllowlistAccessor; import com.linkedin.venice.kafka.protocol.state.PartitionState; @@ -42,10 +44,7 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; -import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; -import com.linkedin.venice.meta.ReadOnlySchemaRepository; -import com.linkedin.venice.meta.ReadOnlyStoreRepository; -import com.linkedin.venice.meta.StaticClusterInfoProvider; +import com.linkedin.venice.meta.*; import com.linkedin.venice.pubsub.PubSubClientsFactory; import com.linkedin.venice.schema.SchemaReader; import com.linkedin.venice.security.SSLFactory; @@ -64,13 +63,13 @@ import com.linkedin.venice.utils.Utils; import com.linkedin.venice.utils.lazy.Lazy; import io.tehuti.metrics.MetricsRepository; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Optional; +import java.util.*; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; +import org.apache.helix.PropertyKey; +import org.apache.helix.model.IdealState; import org.apache.helix.zookeeper.impl.client.ZkClient; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -310,6 +309,9 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; + boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() + || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); + // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( veniceConfigLoader, @@ -317,7 +319,10 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo); + metadataRepo, + whetherToRestoreDataPartitions, + true, + functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -692,6 +697,37 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } + protected final boolean isIsolatedIngestion() { + return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); + } + + private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { + return storageEngineName -> { + String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + + AbstractStorageEngine storageEngine = storageService.getStorageEngine(storageEngineName); + + PropertyKey.Builder propertyKeyBuilder = + new PropertyKey.Builder(this.veniceConfigLoader.getVeniceClusterConfig().getClusterName()); + IdealState idealState = SafeHelixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); + + Set idealStatePartitionIds = new HashSet<>(); + idealState.getPartitionSet().stream().forEach(partitionId -> { + idealStatePartitionIds.add(Integer.parseInt(partitionId)); + }); + Set storageEnginePartitionIds = storageEngine.getPartitionIds(); + + for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + continue; + } + storageEngine.dropPartition(storageEnginePartitionId); + } + + return true; + }; + } + public MetricsRepository getMetricsRepository() { return metricsRepository; } From ff6a16511b18e29a015143fe484664aad649b43e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:43:24 -0700 Subject: [PATCH 06/48] StorageService arguments set to initial state in VeniceServer --- .../com/linkedin/venice/server/VeniceServer.java | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index c8345a32d6..8012adffa3 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -309,9 +309,6 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; - boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() - || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); - // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( veniceConfigLoader, @@ -319,10 +316,7 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo, - whetherToRestoreDataPartitions, - true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + metadataRepo); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -697,10 +691,6 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } - protected final boolean isIsolatedIngestion() { - return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); - } - private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { return storageEngineName -> { String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); From 01ca67faf13e85a577eeb323500613439a123f4e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 24 Sep 2024 00:21:35 -0700 Subject: [PATCH 07/48] Standardize code --- .../kafka/consumer/StoreIngestionTask.java | 13 ------------ .../consumer/StoreIngestionTaskTest.java | 15 ------------- .../store/AbstractStorageEngineTest.java | 21 ------------------- .../rocksdb/RocksDBStorageEngineTest.java | 5 ----- .../linkedin/venice/server/VeniceServer.java | 13 ++++++++++-- 5 files changed, 11 insertions(+), 56 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java index fd08ebb653..bf3e35fb22 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java @@ -586,19 +586,6 @@ void resubscribeForAllPartitions() throws InterruptedException { } } - /** - * Removes partitions that are not subscribed / not assigned. - */ - - public synchronized void removeUnsubscribedPartitions() { - throwIfNotRunning(); - for (PartitionConsumptionState partitionConsumptionState: partitionConsumptionStateMap.values()) { - if (!partitionConsumptionState.isSubscribed()) { - partitionConsumptionStateMap.remove(partitionConsumptionState.getPartition(), partitionConsumptionState); - } - } - } - /** * Adds an asynchronous partition subscription request for the task. */ diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java index 9a78b34f06..a5b5969a80 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java @@ -2034,21 +2034,6 @@ public void testUnsubscribeConsumption(AAConfig aaConfig) throws Exception { }, aaConfig); } - @Test(dataProvider = "aaConfigProvider") - public void testRemoveUnsubscribedPartitions(AAConfig aaConfig) throws Exception { - localVeniceWriter.broadcastStartOfPush(new HashMap<>()); - localVeniceWriter.put(putKeyFoo, putValue, SCHEMA_ID); - - runTest(Utils.setOf(PARTITION_FOO), () -> { - verify(mockLogNotifier, timeout(TEST_TIMEOUT_MS)).started(topic, PARTITION_FOO); - // Start of push has already been consumed. Stop consumption. - storeIngestionTaskUnderTest.unSubscribePartition(fooTopicPartition); - // Unassigned partitions should be removed. - storeIngestionTaskUnderTest.removeUnsubscribedPartitions(); - verify(mockLogNotifier, timeout(TEST_TIMEOUT_MS)).stopped(anyString(), anyInt(), anyLong()); - }, aaConfig); - } - @Test(dataProvider = "aaConfigProvider") public void testKillConsumption(AAConfig aaConfig) throws Exception { final Thread writingThread = new Thread(() -> { diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java index a19a4831a9..cbe8b0117b 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/AbstractStorageEngineTest.java @@ -126,27 +126,6 @@ public void testAddingAPartitionTwice() throws Exception { Assert.fail("Adding the same partition:" + partitionId + " again did not throw any exception as expected."); } - public void testRemovingPartition() throws Exception { - - init(); - - // first, add partition - doAddPartition(partitionId); - - if (!testStore.containsPartition(partitionId)) { - Assert.fail("Adding a new partition: " + partitionId + "failed!"); - } - - // remove existing partition - doRemovePartition(partitionId); - - Assert.assertEquals( - testStoreEngine.containsPartition(partitionId), - false, - "Failed to remove partition: " + partitionId + " from the store engine!"); - - } - public void testRemovingPartitionTwice() throws Exception { init(); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java index f2028d3a14..1af4b08fee 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java @@ -183,11 +183,6 @@ public void testAddingAPartitionTwice() throws Exception { super.testAddingAPartitionTwice(); } - @Test - public void testRemovingPartition() throws Exception { - super.testRemovingPartition(); - } - @Test public void testRemovingPartitionTwice() throws Exception { super.testRemovingPartitionTwice(); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 8012adffa3..da5d3e14f0 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -44,7 +44,11 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; -import com.linkedin.venice.meta.*; +import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; +import com.linkedin.venice.meta.ReadOnlySchemaRepository; +import com.linkedin.venice.meta.ReadOnlyStoreRepository; +import com.linkedin.venice.meta.StaticClusterInfoProvider; +import com.linkedin.venice.meta.Version; import com.linkedin.venice.pubsub.PubSubClientsFactory; import com.linkedin.venice.schema.SchemaReader; import com.linkedin.venice.security.SSLFactory; @@ -63,7 +67,12 @@ import com.linkedin.venice.utils.Utils; import com.linkedin.venice.utils.lazy.Lazy; import io.tehuti.metrics.MetricsRepository; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; From fd145748a3029a571e6e205467e441d73572b5e8 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 24 Sep 2024 00:56:13 -0700 Subject: [PATCH 08/48] Standardize code --- .../davinci/store/rocksdb/RocksDBStoragePartition.java | 4 ---- .../java/com/linkedin/venice/helix/SafeHelixDataAccessor.java | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java index aef4665fc5..87a084dba9 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/rocksdb/RocksDBStoragePartition.java @@ -813,10 +813,6 @@ private void deleteDirectory(String fullPath) { } } - /** - * Note: POI for removing folders for partitions not assigned, for given store_version. - */ - @Override public synchronized void drop() { close(); diff --git a/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java b/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java index f4eb45bc98..afff1b42de 100644 --- a/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java +++ b/internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java @@ -82,7 +82,7 @@ public boolean updateProperty(PropertyKey key, DataUpd return helixDataAccessor.updateProperty(key, updater, value); } - public static T getProperty(PropertyKey key) { + public T getProperty(PropertyKey key) { return helixDataAccessor.getProperty(key); } From 56e77113a9a9018b0c1494e51e8b2bfdc92ed452 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 24 Sep 2024 13:15:14 -0700 Subject: [PATCH 09/48] Update ideal state --- .../linkedin/davinci/helix/HelixParticipationService.java | 4 ++++ .../main/java/com/linkedin/venice/server/VeniceServer.java | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java index 3f04f17dc1..c699b0cd20 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java @@ -427,6 +427,10 @@ public Instance getInstance() { return instance; } + public SafeHelixManager getHelixManager() { + return helixManager; + } + public VeniceOfflinePushMonitorAccessor getVeniceOfflinePushMonitorAccessor() { return veniceOfflinePushMonitorAccessor; } diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index da5d3e14f0..571d120cf3 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -35,7 +35,6 @@ import com.linkedin.venice.helix.HelixCustomizedViewOfflinePushRepository; import com.linkedin.venice.helix.HelixInstanceConfigRepository; import com.linkedin.venice.helix.HelixReadOnlyZKSharedSchemaRepository; -import com.linkedin.venice.helix.SafeHelixDataAccessor; import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.helix.ZkAllowlistAccessor; import com.linkedin.venice.kafka.protocol.state.PartitionState; @@ -708,7 +707,9 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep PropertyKey.Builder propertyKeyBuilder = new PropertyKey.Builder(this.veniceConfigLoader.getVeniceClusterConfig().getClusterName()); - IdealState idealState = SafeHelixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); + IdealState idealState = getHelixParticipationService().getHelixManager() + .getHelixDataAccessor() + .getProperty(propertyKeyBuilder.idealStates(storeName)); Set idealStatePartitionIds = new HashSet<>(); idealState.getPartitionSet().stream().forEach(partitionId -> { From 04c4e9ee0efe1619d628ac4a5fc4d9f6f765d9a2 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 25 Sep 2024 16:09:13 -0700 Subject: [PATCH 10/48] Update initialized StorageService object [with revised import] --- .../com/linkedin/venice/server/VeniceServer.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 571d120cf3..06751375e7 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -43,6 +43,7 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; +import com.linkedin.venice.meta.IngestionMode; import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; import com.linkedin.venice.meta.ReadOnlySchemaRepository; import com.linkedin.venice.meta.ReadOnlyStoreRepository; @@ -317,6 +318,9 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; + boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() + || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); + // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( veniceConfigLoader, @@ -324,7 +328,10 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo); + metadataRepo, + whetherToRestoreDataPartitions, + true, + functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -699,6 +706,10 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } + protected final boolean isIsolatedIngestion() { + return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); + } + private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { return storageEngineName -> { String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); From e09f7bfcbbf67329e8270c75f00cd112da40ad60 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 25 Sep 2024 16:13:54 -0700 Subject: [PATCH 11/48] [Placeholder] --- .../java/com/linkedin/venice/server/VeniceServer.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 06751375e7..1c93211c40 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -714,7 +714,13 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep return storageEngineName -> { String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - AbstractStorageEngine storageEngine = storageService.getStorageEngine(storageEngineName); + StorageEngineRepository storageEngineRepository = new StorageEngineRepository(); + + AbstractStorageEngine storageEngine = storageEngineRepository.getLocalStorageEngine(storageEngineName); + + if (storageEngine == null) { + return true; + } PropertyKey.Builder propertyKeyBuilder = new PropertyKey.Builder(this.veniceConfigLoader.getVeniceClusterConfig().getClusterName()); From 6c505139b4b0d3dc3ca70ced4148da0d9c55ac0d Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 27 Sep 2024 14:33:21 -0700 Subject: [PATCH 12/48] Updated StorageService constructor and initializer with functionToCheckWhetherStoragePartitionShouldBeKeptOrNot --- .../com/linkedin/davinci/DaVinciBackend.java | 3 ++- ...lBootstrappingVeniceChangelogConsumer.java | 7 +++++- .../davinci/storage/StorageService.java | 18 ++++++++++---- .../davinci/storage/StorageServiceTest.java | 1 + .../linkedin/venice/server/VeniceServer.java | 24 ++++++++----------- 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java index 29922abdc8..c6b37d6347 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java @@ -198,7 +198,8 @@ public DaVinciBackend( storeRepository, whetherToRestoreDataPartitions, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot(managedClients)); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot(managedClients), + se -> null); storageService.start(); PubSubClientsFactory pubSubClientsFactory = configLoader.getVeniceServerConfig().getPubSubClientsFactory(); VeniceWriterFactory writerFactory = diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java index 1770c2de9c..34b4e31e47 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java @@ -130,7 +130,8 @@ public InternalLocalBootstrappingVeniceChangelogConsumer( storeRepository, true, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), + functionToCheckWhetherStoragePartitionShouldBeKeptOrNot()); storageMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); } @@ -176,6 +177,10 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep }; } + private Function functionToCheckWhetherStoragePartitionShouldBeKeptOrNot() { + return storageEngine -> null; + } + @Override protected boolean handleVersionSwapControlMessage( ControlMessage controlMessage, diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index f6a92a5add..adc2e9ca8b 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -77,6 +77,7 @@ public class StorageService extends AbstractVeniceService { * @param restoreDataPartitions indicates if store data needs to be restored. * @param restoreMetadataPartitions indicates if meta data needs to be restored. * @param checkWhetherStorageEngineShouldBeKeptOrNot check whether the local storage engine should be kept or not. + * @param checkWhetherStoragePartitionShouldBeKeptOrNot check whether the partition is assigned and thus should be kept or not. */ StorageService( VeniceConfigLoader configLoader, @@ -88,6 +89,7 @@ public class StorageService extends AbstractVeniceService { boolean restoreDataPartitions, boolean restoreMetadataPartitions, Function checkWhetherStorageEngineShouldBeKeptOrNot, + Function checkWhetherStoragePartitionShouldBeKeptOrNot, Optional> persistenceTypeToStorageEngineFactoryMapOptional) { String dataPath = configLoader.getVeniceServerConfig().getDataBasePath(); if (!Utils.directoryExists(dataPath)) { @@ -122,7 +124,8 @@ public class StorageService extends AbstractVeniceService { configLoader, restoreDataPartitions, restoreMetadataPartitions, - checkWhetherStorageEngineShouldBeKeptOrNot); + checkWhetherStorageEngineShouldBeKeptOrNot, + checkWhetherStoragePartitionShouldBeKeptOrNot); } } @@ -135,7 +138,8 @@ public StorageService( ReadOnlyStoreRepository storeRepository, boolean restoreDataPartitions, boolean restoreMetadataPartitions, - Function checkWhetherStorageEngineShouldBeKeptOrNot) { + Function checkWhetherStorageEngineShouldBeKeptOrNot, + Function checkWhetherStoragePartitionShouldBeKeptOrNot) { this( configLoader, storageEngineStats, @@ -146,6 +150,7 @@ public StorageService( restoreDataPartitions, restoreMetadataPartitions, checkWhetherStorageEngineShouldBeKeptOrNot, + checkWhetherStoragePartitionShouldBeKeptOrNot, Optional.empty()); } @@ -167,7 +172,8 @@ public StorageService( storeRepository, restoreDataPartitions, restoreMetadataPartitions, - s -> true); + s -> true, + se -> null); } /** @@ -233,7 +239,8 @@ private void restoreAllStores( VeniceConfigLoader configLoader, boolean restoreDataPartitions, boolean restoreMetadataPartitions, - Function checkWhetherStorageEngineShouldBeKeptOrNot) { + Function checkWhetherStorageEngineShouldBeKeptOrNot, + Function checkWhetherStoragePartitionsShouldBeKeptOrNot) { LOGGER.info("Start restoring all the stores persisted previously"); for (Map.Entry entry: persistenceTypeToStorageEngineFactoryMap.entrySet()) { PersistenceType pType = entry.getKey(); @@ -254,6 +261,7 @@ private void restoreAllStores( if (checkWhetherStorageEngineShouldBeKeptOrNot.apply(storeName)) { try { storageEngine = openStore(storeConfig, () -> null); + checkWhetherStoragePartitionsShouldBeKeptOrNot.apply(storageEngine); } catch (Exception e) { if (ExceptionUtils.recursiveClassEquals(e, RocksDBException.class)) { LOGGER.warn("Encountered RocksDB error while opening store: {}", storeName, e); @@ -467,7 +475,7 @@ public synchronized void closeStorageEngine(String kafkaTopic) { public void cleanupAllStores(VeniceConfigLoader configLoader) { // Load local storage and delete them safely. // TODO Just clean the data dir in case loading and deleting is too slow. - restoreAllStores(configLoader, true, true, s -> true); + restoreAllStores(configLoader, true, true, s -> true, se -> null); LOGGER.info("Start cleaning up all the stores persisted previously"); storageEngineRepository.getAllLocalStorageEngines().stream().forEach(storageEngine -> { String storeName = storageEngine.getStoreVersionName(); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index a86a4879be..d8dddce5fe 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -115,6 +115,7 @@ public void testGetStoreAndUserPartitionsMapping() { true, true, (s) -> true, + (se) -> null, Optional.of(persistenceTypeToStorageEngineFactoryMap)); Map> expectedMapping = new HashMap<>(); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 1c93211c40..220c872397 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -331,7 +331,8 @@ private List createServices() { metadataRepo, whetherToRestoreDataPartitions, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), + functionToCheckWhetherStoragePartitionShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -711,19 +712,15 @@ protected final boolean isIsolatedIngestion() { } private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { - return storageEngineName -> { - String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - - StorageEngineRepository storageEngineRepository = new StorageEngineRepository(); - - AbstractStorageEngine storageEngine = storageEngineRepository.getLocalStorageEngine(storageEngineName); - - if (storageEngine == null) { - return true; - } + return storageEngineName -> true; + } + private Function functionToCheckWhetherStoragePartitionShouldBeKeptOrNot() { + return storageEngine -> { + String storageEngineName = storageEngine.toString(); + String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); PropertyKey.Builder propertyKeyBuilder = - new PropertyKey.Builder(this.veniceConfigLoader.getVeniceClusterConfig().getClusterName()); + new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); IdealState idealState = getHelixParticipationService().getHelixManager() .getHelixDataAccessor() .getProperty(propertyKeyBuilder.idealStates(storeName)); @@ -740,8 +737,7 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep } storageEngine.dropPartition(storageEnginePartitionId); } - - return true; + return null; }; } From 06dd6c242a7bddea6a1fef2e819d7cb3f80f294f Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 27 Sep 2024 14:45:20 -0700 Subject: [PATCH 13/48] Updated StorageService constructor and initializer [modified] --- ...ernalLocalBootstrappingVeniceChangelogConsumer.java | 4 ++-- .../com/linkedin/davinci/storage/StorageService.java | 10 +++++----- .../java/com/linkedin/venice/server/VeniceServer.java | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java index 34b4e31e47..bc06d839d3 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java @@ -131,7 +131,7 @@ public InternalLocalBootstrappingVeniceChangelogConsumer( true, true, functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), - functionToCheckWhetherStoragePartitionShouldBeKeptOrNot()); + functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot()); storageMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); } @@ -177,7 +177,7 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep }; } - private Function functionToCheckWhetherStoragePartitionShouldBeKeptOrNot() { + private Function functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot() { return storageEngine -> null; } diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index adc2e9ca8b..1eb97232cc 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -77,7 +77,7 @@ public class StorageService extends AbstractVeniceService { * @param restoreDataPartitions indicates if store data needs to be restored. * @param restoreMetadataPartitions indicates if meta data needs to be restored. * @param checkWhetherStorageEngineShouldBeKeptOrNot check whether the local storage engine should be kept or not. - * @param checkWhetherStoragePartitionShouldBeKeptOrNot check whether the partition is assigned and thus should be kept or not. + * @param checkWhetherStoragePartitionsShouldBeKeptOrNot check whether the partition is assigned and thus should be kept or not. */ StorageService( VeniceConfigLoader configLoader, @@ -89,7 +89,7 @@ public class StorageService extends AbstractVeniceService { boolean restoreDataPartitions, boolean restoreMetadataPartitions, Function checkWhetherStorageEngineShouldBeKeptOrNot, - Function checkWhetherStoragePartitionShouldBeKeptOrNot, + Function checkWhetherStoragePartitionsShouldBeKeptOrNot, Optional> persistenceTypeToStorageEngineFactoryMapOptional) { String dataPath = configLoader.getVeniceServerConfig().getDataBasePath(); if (!Utils.directoryExists(dataPath)) { @@ -125,7 +125,7 @@ public class StorageService extends AbstractVeniceService { restoreDataPartitions, restoreMetadataPartitions, checkWhetherStorageEngineShouldBeKeptOrNot, - checkWhetherStoragePartitionShouldBeKeptOrNot); + checkWhetherStoragePartitionsShouldBeKeptOrNot); } } @@ -139,7 +139,7 @@ public StorageService( boolean restoreDataPartitions, boolean restoreMetadataPartitions, Function checkWhetherStorageEngineShouldBeKeptOrNot, - Function checkWhetherStoragePartitionShouldBeKeptOrNot) { + Function checkWhetherStoragePartitionsShouldBeKeptOrNot) { this( configLoader, storageEngineStats, @@ -150,7 +150,7 @@ public StorageService( restoreDataPartitions, restoreMetadataPartitions, checkWhetherStorageEngineShouldBeKeptOrNot, - checkWhetherStoragePartitionShouldBeKeptOrNot, + checkWhetherStoragePartitionsShouldBeKeptOrNot, Optional.empty()); } diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 220c872397..27dbf3b854 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -332,7 +332,7 @@ private List createServices() { whetherToRestoreDataPartitions, true, functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), - functionToCheckWhetherStoragePartitionShouldBeKeptOrNot()); + functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -715,7 +715,7 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep return storageEngineName -> true; } - private Function functionToCheckWhetherStoragePartitionShouldBeKeptOrNot() { + private Function functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot() { return storageEngine -> { String storageEngineName = storageEngine.toString(); String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); From 53d177c824422791b47cfc0428b08ed337cc406d Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:24:46 -0700 Subject: [PATCH 14/48] Revised StorageService constructor --- .../com/linkedin/davinci/DaVinciBackend.java | 3 +- ...lBootstrappingVeniceChangelogConsumer.java | 7 +- .../davinci/storage/StorageService.java | 87 ++++++++++++++++++- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java index c6b37d6347..29922abdc8 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/DaVinciBackend.java @@ -198,8 +198,7 @@ public DaVinciBackend( storeRepository, whetherToRestoreDataPartitions, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot(managedClients), - se -> null); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot(managedClients)); storageService.start(); PubSubClientsFactory pubSubClientsFactory = configLoader.getVeniceServerConfig().getPubSubClientsFactory(); VeniceWriterFactory writerFactory = diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java index bc06d839d3..1770c2de9c 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java @@ -130,8 +130,7 @@ public InternalLocalBootstrappingVeniceChangelogConsumer( storeRepository, true, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), - functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot()); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); } @@ -177,10 +176,6 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep }; } - private Function functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot() { - return storageEngine -> null; - } - @Override protected boolean handleVersionSwapControlMessage( ControlMessage controlMessage, diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 1eb97232cc..9721b019d2 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -129,6 +129,67 @@ public class StorageService extends AbstractVeniceService { } } + /** + * Allocates a new {@code StorageService} object. + * @param configLoader a config loader to load configs related to cluster and server. + * @param storageEngineStats storage engine related stats. + * @param rocksDBMemoryStats RocksDB memory consumption stats. + * @param storeVersionStateSerializer serializer for translating a store-version level state into avro-format. + * @param partitionStateSerializer serializer for translating a partition state into avro-format. + * @param storeRepository supports readonly operations to access stores + * @param restoreDataPartitions indicates if store data needs to be restored. + * @param restoreMetadataPartitions indicates if meta data needs to be restored. + * @param checkWhetherStorageEngineShouldBeKeptOrNot check whether the local storage engine should be kept or not. + */ + StorageService( + VeniceConfigLoader configLoader, + AggVersionedStorageEngineStats storageEngineStats, + RocksDBMemoryStats rocksDBMemoryStats, + InternalAvroSpecificSerializer storeVersionStateSerializer, + InternalAvroSpecificSerializer partitionStateSerializer, + ReadOnlyStoreRepository storeRepository, + boolean restoreDataPartitions, + boolean restoreMetadataPartitions, + Function checkWhetherStorageEngineShouldBeKeptOrNot, + Optional> persistenceTypeToStorageEngineFactoryMapOptional) { + String dataPath = configLoader.getVeniceServerConfig().getDataBasePath(); + if (!Utils.directoryExists(dataPath)) { + if (!configLoader.getVeniceServerConfig().isAutoCreateDataPath()) { + throw new VeniceException( + "Data directory '" + dataPath + "' does not exist and " + ConfigKeys.AUTOCREATE_DATA_PATH + + " is disabled."); + } + + File dataDir = new File(dataPath); + LOGGER.info("Creating data directory {}", dataDir.getAbsolutePath()); + dataDir.mkdirs(); + } + + this.configLoader = configLoader; + this.serverConfig = configLoader.getVeniceServerConfig(); + this.storageEngineRepository = new StorageEngineRepository(); + + this.aggVersionedStorageEngineStats = storageEngineStats; + this.rocksDBMemoryStats = rocksDBMemoryStats; + this.storeVersionStateSerializer = storeVersionStateSerializer; + this.partitionStateSerializer = partitionStateSerializer; + this.storeRepository = storeRepository; + if (persistenceTypeToStorageEngineFactoryMapOptional.isPresent()) { + this.persistenceTypeToStorageEngineFactoryMap = persistenceTypeToStorageEngineFactoryMapOptional.get(); + } else { + this.persistenceTypeToStorageEngineFactoryMap = new HashMap<>(); + initInternalStorageEngineFactories(); + } + if (restoreDataPartitions || restoreMetadataPartitions) { + restoreAllStores( + configLoader, + restoreDataPartitions, + restoreMetadataPartitions, + checkWhetherStorageEngineShouldBeKeptOrNot, + se -> null); + } + } + public StorageService( VeniceConfigLoader configLoader, AggVersionedStorageEngineStats storageEngineStats, @@ -154,6 +215,29 @@ public StorageService( Optional.empty()); } + public StorageService( + VeniceConfigLoader configLoader, + AggVersionedStorageEngineStats storageEngineStats, + RocksDBMemoryStats rocksDBMemoryStats, + InternalAvroSpecificSerializer storeVersionStateSerializer, + InternalAvroSpecificSerializer partitionStateSerializer, + ReadOnlyStoreRepository storeRepository, + boolean restoreDataPartitions, + boolean restoreMetadataPartitions, + Function checkWhetherStorageEngineShouldBeKeptOrNot) { + this( + configLoader, + storageEngineStats, + rocksDBMemoryStats, + storeVersionStateSerializer, + partitionStateSerializer, + storeRepository, + restoreDataPartitions, + restoreMetadataPartitions, + checkWhetherStorageEngineShouldBeKeptOrNot, + Optional.empty()); + } + public StorageService( VeniceConfigLoader configLoader, AggVersionedStorageEngineStats storageEngineStats, @@ -172,8 +256,7 @@ public StorageService( storeRepository, restoreDataPartitions, restoreMetadataPartitions, - s -> true, - se -> null); + s -> true); } /** From df1c0b9f3b19a7dc4fbd3113512e3a21abbc278f Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:58:26 -0700 Subject: [PATCH 15/48] Code restructure: verifying storage partition --- .../davinci/storage/StorageService.java | 97 +------------------ .../davinci/storage/StorageServiceTest.java | 1 - .../linkedin/venice/server/VeniceServer.java | 7 +- 3 files changed, 8 insertions(+), 97 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 9721b019d2..f6a92a5add 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -77,7 +77,6 @@ public class StorageService extends AbstractVeniceService { * @param restoreDataPartitions indicates if store data needs to be restored. * @param restoreMetadataPartitions indicates if meta data needs to be restored. * @param checkWhetherStorageEngineShouldBeKeptOrNot check whether the local storage engine should be kept or not. - * @param checkWhetherStoragePartitionsShouldBeKeptOrNot check whether the partition is assigned and thus should be kept or not. */ StorageService( VeniceConfigLoader configLoader, @@ -89,7 +88,6 @@ public class StorageService extends AbstractVeniceService { boolean restoreDataPartitions, boolean restoreMetadataPartitions, Function checkWhetherStorageEngineShouldBeKeptOrNot, - Function checkWhetherStoragePartitionsShouldBeKeptOrNot, Optional> persistenceTypeToStorageEngineFactoryMapOptional) { String dataPath = configLoader.getVeniceServerConfig().getDataBasePath(); if (!Utils.directoryExists(dataPath)) { @@ -124,97 +122,10 @@ public class StorageService extends AbstractVeniceService { configLoader, restoreDataPartitions, restoreMetadataPartitions, - checkWhetherStorageEngineShouldBeKeptOrNot, - checkWhetherStoragePartitionsShouldBeKeptOrNot); + checkWhetherStorageEngineShouldBeKeptOrNot); } } - /** - * Allocates a new {@code StorageService} object. - * @param configLoader a config loader to load configs related to cluster and server. - * @param storageEngineStats storage engine related stats. - * @param rocksDBMemoryStats RocksDB memory consumption stats. - * @param storeVersionStateSerializer serializer for translating a store-version level state into avro-format. - * @param partitionStateSerializer serializer for translating a partition state into avro-format. - * @param storeRepository supports readonly operations to access stores - * @param restoreDataPartitions indicates if store data needs to be restored. - * @param restoreMetadataPartitions indicates if meta data needs to be restored. - * @param checkWhetherStorageEngineShouldBeKeptOrNot check whether the local storage engine should be kept or not. - */ - StorageService( - VeniceConfigLoader configLoader, - AggVersionedStorageEngineStats storageEngineStats, - RocksDBMemoryStats rocksDBMemoryStats, - InternalAvroSpecificSerializer storeVersionStateSerializer, - InternalAvroSpecificSerializer partitionStateSerializer, - ReadOnlyStoreRepository storeRepository, - boolean restoreDataPartitions, - boolean restoreMetadataPartitions, - Function checkWhetherStorageEngineShouldBeKeptOrNot, - Optional> persistenceTypeToStorageEngineFactoryMapOptional) { - String dataPath = configLoader.getVeniceServerConfig().getDataBasePath(); - if (!Utils.directoryExists(dataPath)) { - if (!configLoader.getVeniceServerConfig().isAutoCreateDataPath()) { - throw new VeniceException( - "Data directory '" + dataPath + "' does not exist and " + ConfigKeys.AUTOCREATE_DATA_PATH - + " is disabled."); - } - - File dataDir = new File(dataPath); - LOGGER.info("Creating data directory {}", dataDir.getAbsolutePath()); - dataDir.mkdirs(); - } - - this.configLoader = configLoader; - this.serverConfig = configLoader.getVeniceServerConfig(); - this.storageEngineRepository = new StorageEngineRepository(); - - this.aggVersionedStorageEngineStats = storageEngineStats; - this.rocksDBMemoryStats = rocksDBMemoryStats; - this.storeVersionStateSerializer = storeVersionStateSerializer; - this.partitionStateSerializer = partitionStateSerializer; - this.storeRepository = storeRepository; - if (persistenceTypeToStorageEngineFactoryMapOptional.isPresent()) { - this.persistenceTypeToStorageEngineFactoryMap = persistenceTypeToStorageEngineFactoryMapOptional.get(); - } else { - this.persistenceTypeToStorageEngineFactoryMap = new HashMap<>(); - initInternalStorageEngineFactories(); - } - if (restoreDataPartitions || restoreMetadataPartitions) { - restoreAllStores( - configLoader, - restoreDataPartitions, - restoreMetadataPartitions, - checkWhetherStorageEngineShouldBeKeptOrNot, - se -> null); - } - } - - public StorageService( - VeniceConfigLoader configLoader, - AggVersionedStorageEngineStats storageEngineStats, - RocksDBMemoryStats rocksDBMemoryStats, - InternalAvroSpecificSerializer storeVersionStateSerializer, - InternalAvroSpecificSerializer partitionStateSerializer, - ReadOnlyStoreRepository storeRepository, - boolean restoreDataPartitions, - boolean restoreMetadataPartitions, - Function checkWhetherStorageEngineShouldBeKeptOrNot, - Function checkWhetherStoragePartitionsShouldBeKeptOrNot) { - this( - configLoader, - storageEngineStats, - rocksDBMemoryStats, - storeVersionStateSerializer, - partitionStateSerializer, - storeRepository, - restoreDataPartitions, - restoreMetadataPartitions, - checkWhetherStorageEngineShouldBeKeptOrNot, - checkWhetherStoragePartitionsShouldBeKeptOrNot, - Optional.empty()); - } - public StorageService( VeniceConfigLoader configLoader, AggVersionedStorageEngineStats storageEngineStats, @@ -322,8 +233,7 @@ private void restoreAllStores( VeniceConfigLoader configLoader, boolean restoreDataPartitions, boolean restoreMetadataPartitions, - Function checkWhetherStorageEngineShouldBeKeptOrNot, - Function checkWhetherStoragePartitionsShouldBeKeptOrNot) { + Function checkWhetherStorageEngineShouldBeKeptOrNot) { LOGGER.info("Start restoring all the stores persisted previously"); for (Map.Entry entry: persistenceTypeToStorageEngineFactoryMap.entrySet()) { PersistenceType pType = entry.getKey(); @@ -344,7 +254,6 @@ private void restoreAllStores( if (checkWhetherStorageEngineShouldBeKeptOrNot.apply(storeName)) { try { storageEngine = openStore(storeConfig, () -> null); - checkWhetherStoragePartitionsShouldBeKeptOrNot.apply(storageEngine); } catch (Exception e) { if (ExceptionUtils.recursiveClassEquals(e, RocksDBException.class)) { LOGGER.warn("Encountered RocksDB error while opening store: {}", storeName, e); @@ -558,7 +467,7 @@ public synchronized void closeStorageEngine(String kafkaTopic) { public void cleanupAllStores(VeniceConfigLoader configLoader) { // Load local storage and delete them safely. // TODO Just clean the data dir in case loading and deleting is too slow. - restoreAllStores(configLoader, true, true, s -> true, se -> null); + restoreAllStores(configLoader, true, true, s -> true); LOGGER.info("Start cleaning up all the stores persisted previously"); storageEngineRepository.getAllLocalStorageEngines().stream().forEach(storageEngine -> { String storeName = storageEngine.getStoreVersionName(); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index d8dddce5fe..a86a4879be 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -115,7 +115,6 @@ public void testGetStoreAndUserPartitionsMapping() { true, true, (s) -> true, - (se) -> null, Optional.of(persistenceTypeToStorageEngineFactoryMap)); Map> expectedMapping = new HashMap<>(); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 676b402b61..a21697b896 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -331,8 +331,7 @@ private List createServices() { metadataRepo, whetherToRestoreDataPartitions, true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot(), - functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot()); + functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -601,6 +600,10 @@ public void start() throws VeniceException { service.start(); } + for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository().getAllLocalStorageEngines()) { + functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot().apply(storageEngine); + } + for (ServiceDiscoveryAnnouncer serviceDiscoveryAnnouncer: serviceDiscoveryAnnouncers) { LOGGER.info("Registering to service discovery: {}", serviceDiscoveryAnnouncer); serviceDiscoveryAnnouncer.register(); From 54d870867bac5255f9103d417ab08af99ae26637 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 1 Oct 2024 13:00:34 -0700 Subject: [PATCH 16/48] Code restructure: move storage partition check to AbstractStorageEngine --- .../davinci/store/AbstractStorageEngine.java | 29 +++++++++++++ .../ServerReadMetadataRepository.java | 1 + .../linkedin/venice/server/VeniceServer.java | 43 ++++--------------- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index 1562c71469..15cc714ce1 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -5,13 +5,16 @@ import static com.linkedin.davinci.store.AbstractStorageEngine.StoragePartitionAdjustmentTrigger.END_BATCH_PUSH; import com.linkedin.davinci.callback.BytesStreamingCallback; +import com.linkedin.davinci.config.VeniceConfigLoader; import com.linkedin.venice.compression.CompressionStrategy; import com.linkedin.venice.exceptions.PersistenceFailureException; import com.linkedin.venice.exceptions.StorageInitializationException; import com.linkedin.venice.exceptions.VeniceException; +import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PersistenceType; +import com.linkedin.venice.meta.Version; import com.linkedin.venice.offsets.OffsetRecord; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.utils.LatencyUtils; @@ -20,6 +23,7 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -30,6 +34,8 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import java.util.stream.Collectors; +import org.apache.helix.PropertyKey; +import org.apache.helix.model.IdealState; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -246,6 +252,29 @@ public synchronized void addStoragePartition(StoragePartitionConfig storageParti } } + public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( + SafeHelixManager manager, + VeniceConfigLoader veniceConfigLoader) { + String storageEngineName = getStoreVersionName(); + String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + PropertyKey.Builder propertyKeyBuilder = + new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); + IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); + + Set idealStatePartitionIds = new HashSet<>(); + idealState.getPartitionSet().stream().forEach(partitionId -> { + idealStatePartitionIds.add(Integer.parseInt(partitionId)); + }); + Set storageEnginePartitionIds = getPartitionIds(); + + for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + continue; + } + dropPartition(storageEnginePartitionId); + } + } + public synchronized void closePartition(int partitionId) { AbstractStoragePartition partition = this.partitionList.remove(partitionId); if (partition == null) { diff --git a/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java b/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java index a6126fdd43..50950a4531 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java @@ -59,6 +59,7 @@ public ServerReadMetadataRepository( customizedViewFuture.ifPresent(future -> future.thenApply(cv -> this.customizedViewRepository = cv)); helixInstanceFuture.ifPresent(future -> future.thenApply(helix -> this.helixInstanceConfigRepository = helix)); + } /** diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index a21697b896..a4d10b0cf5 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -48,7 +48,6 @@ import com.linkedin.venice.meta.ReadOnlySchemaRepository; import com.linkedin.venice.meta.ReadOnlyStoreRepository; import com.linkedin.venice.meta.StaticClusterInfoProvider; -import com.linkedin.venice.meta.Version; import com.linkedin.venice.pubsub.PubSubClientsFactory; import com.linkedin.venice.schema.SchemaReader; import com.linkedin.venice.security.SSLFactory; @@ -69,16 +68,12 @@ import io.tehuti.metrics.MetricsRepository; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; -import org.apache.helix.PropertyKey; -import org.apache.helix.model.IdealState; import org.apache.helix.zookeeper.impl.client.ZkClient; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -369,6 +364,14 @@ private List createServices() { return helixData; }); + managerFuture.thenApply(manager -> { + for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() + .getAllLocalStorageEngines()) { + storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); + } + return true; + }); + heartbeatMonitoringService = new HeartbeatMonitoringService( metricsRepository, metadataRepo, @@ -600,10 +603,6 @@ public void start() throws VeniceException { service.start(); } - for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository().getAllLocalStorageEngines()) { - functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot().apply(storageEngine); - } - for (ServiceDiscoveryAnnouncer serviceDiscoveryAnnouncer: serviceDiscoveryAnnouncers) { LOGGER.info("Registering to service discovery: {}", serviceDiscoveryAnnouncer); serviceDiscoveryAnnouncer.register(); @@ -719,32 +718,6 @@ private Function functionToCheckWhetherStorageEngineShouldBeKep return storageEngineName -> true; } - private Function functionToCheckWhetherStoragePartitionsShouldBeKeptOrNot() { - return storageEngine -> { - String storageEngineName = storageEngine.toString(); - String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - PropertyKey.Builder propertyKeyBuilder = - new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); - IdealState idealState = getHelixParticipationService().getHelixManager() - .getHelixDataAccessor() - .getProperty(propertyKeyBuilder.idealStates(storeName)); - - Set idealStatePartitionIds = new HashSet<>(); - idealState.getPartitionSet().stream().forEach(partitionId -> { - idealStatePartitionIds.add(Integer.parseInt(partitionId)); - }); - Set storageEnginePartitionIds = storageEngine.getPartitionIds(); - - for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - if (idealStatePartitionIds.contains(storageEnginePartitionId)) { - continue; - } - storageEngine.dropPartition(storageEnginePartitionId); - } - return null; - }; - } - public MetricsRepository getMetricsRepository() { return metricsRepository; } From eb5dd8909bdb0fc43b99fe9cfbb467fa4132a60a Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 1 Oct 2024 18:59:27 -0700 Subject: [PATCH 17/48] [Commented modified code] --- .../davinci/store/AbstractStorageEngine.java | 50 ++++++++----------- .../linkedin/venice/server/VeniceServer.java | 43 ++++++++-------- 2 files changed, 42 insertions(+), 51 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index 15cc714ce1..d400f08159 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -5,16 +5,13 @@ import static com.linkedin.davinci.store.AbstractStorageEngine.StoragePartitionAdjustmentTrigger.END_BATCH_PUSH; import com.linkedin.davinci.callback.BytesStreamingCallback; -import com.linkedin.davinci.config.VeniceConfigLoader; import com.linkedin.venice.compression.CompressionStrategy; import com.linkedin.venice.exceptions.PersistenceFailureException; import com.linkedin.venice.exceptions.StorageInitializationException; import com.linkedin.venice.exceptions.VeniceException; -import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PersistenceType; -import com.linkedin.venice.meta.Version; import com.linkedin.venice.offsets.OffsetRecord; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.utils.LatencyUtils; @@ -23,7 +20,6 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -34,8 +30,6 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.apache.helix.PropertyKey; -import org.apache.helix.model.IdealState; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -252,28 +246,28 @@ public synchronized void addStoragePartition(StoragePartitionConfig storageParti } } - public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( - SafeHelixManager manager, - VeniceConfigLoader veniceConfigLoader) { - String storageEngineName = getStoreVersionName(); - String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - PropertyKey.Builder propertyKeyBuilder = - new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); - IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); - - Set idealStatePartitionIds = new HashSet<>(); - idealState.getPartitionSet().stream().forEach(partitionId -> { - idealStatePartitionIds.add(Integer.parseInt(partitionId)); - }); - Set storageEnginePartitionIds = getPartitionIds(); - - for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - if (idealStatePartitionIds.contains(storageEnginePartitionId)) { - continue; - } - dropPartition(storageEnginePartitionId); - } - } + // public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( + // SafeHelixManager manager, + // VeniceConfigLoader veniceConfigLoader) { + // String storageEngineName = getStoreVersionName(); + // String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + // PropertyKey.Builder propertyKeyBuilder = + // new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); + // IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); + // + // Set idealStatePartitionIds = new HashSet<>(); + // idealState.getPartitionSet().stream().forEach(partitionId -> { + // idealStatePartitionIds.add(Integer.parseInt(partitionId)); + // }); + // Set storageEnginePartitionIds = getPartitionIds(); + // + // for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + // if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + // continue; + // } + // dropPartition(storageEnginePartitionId); + // } + // } public synchronized void closePartition(int partitionId) { AbstractStoragePartition partition = this.partitionList.remove(partitionId); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index a4d10b0cf5..822e8440cd 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -21,7 +21,6 @@ import com.linkedin.davinci.storage.StorageEngineRepository; import com.linkedin.davinci.storage.StorageMetadataService; import com.linkedin.davinci.storage.StorageService; -import com.linkedin.davinci.store.AbstractStorageEngine; import com.linkedin.venice.acl.DynamicAccessController; import com.linkedin.venice.acl.StaticAccessController; import com.linkedin.venice.cleaner.BackupVersionOptimizationService; @@ -43,7 +42,6 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; -import com.linkedin.venice.meta.IngestionMode; import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; import com.linkedin.venice.meta.ReadOnlySchemaRepository; import com.linkedin.venice.meta.ReadOnlyStoreRepository; @@ -73,7 +71,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Function; import org.apache.helix.zookeeper.impl.client.ZkClient; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -313,8 +310,8 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; - boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() - || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); + // boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() + // || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( @@ -323,10 +320,10 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo, - whetherToRestoreDataPartitions, - true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + metadataRepo); + // whetherToRestoreDataPartitions, + // true, + // functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -364,13 +361,13 @@ private List createServices() { return helixData; }); - managerFuture.thenApply(manager -> { - for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() - .getAllLocalStorageEngines()) { - storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); - } - return true; - }); + // managerFuture.thenApply(manager -> { + // for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() + // .getAllLocalStorageEngines()) { + // storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); + // } + // return true; + // }); heartbeatMonitoringService = new HeartbeatMonitoringService( metricsRepository, @@ -710,13 +707,13 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } - protected final boolean isIsolatedIngestion() { - return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); - } - - private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { - return storageEngineName -> true; - } + // protected final boolean isIsolatedIngestion() { + // return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); + // } + // + // private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { + // return storageEngineName -> true; + // } public MetricsRepository getMetricsRepository() { return metricsRepository; From d55f7049cdf1bf83853d243e37eed8d463db93c5 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 1 Oct 2024 21:39:19 -0700 Subject: [PATCH 18/48] Code restructure: move storage partition check to AbstractStorageEngine --- .../davinci/store/AbstractStorageEngine.java | 50 +++++++++++-------- .../linkedin/venice/server/VeniceServer.java | 43 ++++++++-------- 2 files changed, 51 insertions(+), 42 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index d400f08159..15cc714ce1 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -5,13 +5,16 @@ import static com.linkedin.davinci.store.AbstractStorageEngine.StoragePartitionAdjustmentTrigger.END_BATCH_PUSH; import com.linkedin.davinci.callback.BytesStreamingCallback; +import com.linkedin.davinci.config.VeniceConfigLoader; import com.linkedin.venice.compression.CompressionStrategy; import com.linkedin.venice.exceptions.PersistenceFailureException; import com.linkedin.venice.exceptions.StorageInitializationException; import com.linkedin.venice.exceptions.VeniceException; +import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PersistenceType; +import com.linkedin.venice.meta.Version; import com.linkedin.venice.offsets.OffsetRecord; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.utils.LatencyUtils; @@ -20,6 +23,7 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -30,6 +34,8 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import java.util.stream.Collectors; +import org.apache.helix.PropertyKey; +import org.apache.helix.model.IdealState; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -246,28 +252,28 @@ public synchronized void addStoragePartition(StoragePartitionConfig storageParti } } - // public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( - // SafeHelixManager manager, - // VeniceConfigLoader veniceConfigLoader) { - // String storageEngineName = getStoreVersionName(); - // String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - // PropertyKey.Builder propertyKeyBuilder = - // new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); - // IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); - // - // Set idealStatePartitionIds = new HashSet<>(); - // idealState.getPartitionSet().stream().forEach(partitionId -> { - // idealStatePartitionIds.add(Integer.parseInt(partitionId)); - // }); - // Set storageEnginePartitionIds = getPartitionIds(); - // - // for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - // if (idealStatePartitionIds.contains(storageEnginePartitionId)) { - // continue; - // } - // dropPartition(storageEnginePartitionId); - // } - // } + public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( + SafeHelixManager manager, + VeniceConfigLoader veniceConfigLoader) { + String storageEngineName = getStoreVersionName(); + String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + PropertyKey.Builder propertyKeyBuilder = + new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); + IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); + + Set idealStatePartitionIds = new HashSet<>(); + idealState.getPartitionSet().stream().forEach(partitionId -> { + idealStatePartitionIds.add(Integer.parseInt(partitionId)); + }); + Set storageEnginePartitionIds = getPartitionIds(); + + for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + continue; + } + dropPartition(storageEnginePartitionId); + } + } public synchronized void closePartition(int partitionId) { AbstractStoragePartition partition = this.partitionList.remove(partitionId); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 822e8440cd..a4d10b0cf5 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -21,6 +21,7 @@ import com.linkedin.davinci.storage.StorageEngineRepository; import com.linkedin.davinci.storage.StorageMetadataService; import com.linkedin.davinci.storage.StorageService; +import com.linkedin.davinci.store.AbstractStorageEngine; import com.linkedin.venice.acl.DynamicAccessController; import com.linkedin.venice.acl.StaticAccessController; import com.linkedin.venice.cleaner.BackupVersionOptimizationService; @@ -42,6 +43,7 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; +import com.linkedin.venice.meta.IngestionMode; import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; import com.linkedin.venice.meta.ReadOnlySchemaRepository; import com.linkedin.venice.meta.ReadOnlyStoreRepository; @@ -71,6 +73,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; import org.apache.helix.zookeeper.impl.client.ZkClient; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -310,8 +313,8 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; - // boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() - // || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); + boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() + || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( @@ -320,10 +323,10 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo); - // whetherToRestoreDataPartitions, - // true, - // functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + metadataRepo, + whetherToRestoreDataPartitions, + true, + functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -361,13 +364,13 @@ private List createServices() { return helixData; }); - // managerFuture.thenApply(manager -> { - // for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() - // .getAllLocalStorageEngines()) { - // storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); - // } - // return true; - // }); + managerFuture.thenApply(manager -> { + for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() + .getAllLocalStorageEngines()) { + storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); + } + return true; + }); heartbeatMonitoringService = new HeartbeatMonitoringService( metricsRepository, @@ -707,13 +710,13 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } - // protected final boolean isIsolatedIngestion() { - // return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); - // } - // - // private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { - // return storageEngineName -> true; - // } + protected final boolean isIsolatedIngestion() { + return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); + } + + private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { + return storageEngineName -> true; + } public MetricsRepository getMetricsRepository() { return metricsRepository; From 3ffd26d577687554fcaae573b219b4573c4bba94 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 2 Oct 2024 11:33:59 -0700 Subject: [PATCH 19/48] Code restructure --- .../davinci/storage/StorageService.java | 27 +++++++++++++++++ .../davinci/store/AbstractStorageEngine.java | 29 ------------------- .../linkedin/venice/server/VeniceServer.java | 6 +--- 3 files changed, 28 insertions(+), 34 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index f6a92a5add..1efd1bd428 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -17,6 +17,7 @@ import com.linkedin.venice.ConfigKeys; import com.linkedin.venice.exceptions.VeniceException; import com.linkedin.venice.exceptions.VeniceNoStoreException; +import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PersistenceType; @@ -42,6 +43,8 @@ import java.util.function.BiConsumer; import java.util.function.Function; import java.util.function.Supplier; +import org.apache.helix.PropertyKey; +import org.apache.helix.model.IdealState; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.rocksdb.RocksDBException; @@ -371,6 +374,30 @@ public synchronized AbstractStorageEngine openStore( return engine; } + public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHelixManager manager) { + for (AbstractStorageEngine storageEngine: getStorageEngineRepository().getAllLocalStorageEngines()) { + String storageEngineName = storageEngine.toString(); + String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + PropertyKey.Builder propertyKeyBuilder = + new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); + IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); + + Set idealStatePartitionIds = new HashSet<>(); + + idealState.getPartitionSet().stream().forEach(partitionId -> { + idealStatePartitionIds.add(Integer.parseInt(partitionId)); + }); + Set storageEnginePartitionIds = storageEngine.getPartitionIds(); + + for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + continue; + } + storageEngine.dropPartition(storageEnginePartitionId); + } + } + } + /** * Drops the partition of the specified store version in the storage service. When all data partitions are dropped, * it will also drop the storage engine of the specific store version. diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index 15cc714ce1..1562c71469 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -5,16 +5,13 @@ import static com.linkedin.davinci.store.AbstractStorageEngine.StoragePartitionAdjustmentTrigger.END_BATCH_PUSH; import com.linkedin.davinci.callback.BytesStreamingCallback; -import com.linkedin.davinci.config.VeniceConfigLoader; import com.linkedin.venice.compression.CompressionStrategy; import com.linkedin.venice.exceptions.PersistenceFailureException; import com.linkedin.venice.exceptions.StorageInitializationException; import com.linkedin.venice.exceptions.VeniceException; -import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PersistenceType; -import com.linkedin.venice.meta.Version; import com.linkedin.venice.offsets.OffsetRecord; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.utils.LatencyUtils; @@ -23,7 +20,6 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -34,8 +30,6 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.apache.helix.PropertyKey; -import org.apache.helix.model.IdealState; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -252,29 +246,6 @@ public synchronized void addStoragePartition(StoragePartitionConfig storageParti } } - public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot( - SafeHelixManager manager, - VeniceConfigLoader veniceConfigLoader) { - String storageEngineName = getStoreVersionName(); - String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - PropertyKey.Builder propertyKeyBuilder = - new PropertyKey.Builder(veniceConfigLoader.getVeniceClusterConfig().getClusterName()); - IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); - - Set idealStatePartitionIds = new HashSet<>(); - idealState.getPartitionSet().stream().forEach(partitionId -> { - idealStatePartitionIds.add(Integer.parseInt(partitionId)); - }); - Set storageEnginePartitionIds = getPartitionIds(); - - for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - if (idealStatePartitionIds.contains(storageEnginePartitionId)) { - continue; - } - dropPartition(storageEnginePartitionId); - } - } - public synchronized void closePartition(int partitionId) { AbstractStoragePartition partition = this.partitionList.remove(partitionId); if (partition == null) { diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index a4d10b0cf5..3727af035b 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -21,7 +21,6 @@ import com.linkedin.davinci.storage.StorageEngineRepository; import com.linkedin.davinci.storage.StorageMetadataService; import com.linkedin.davinci.storage.StorageService; -import com.linkedin.davinci.store.AbstractStorageEngine; import com.linkedin.venice.acl.DynamicAccessController; import com.linkedin.venice.acl.StaticAccessController; import com.linkedin.venice.cleaner.BackupVersionOptimizationService; @@ -365,10 +364,7 @@ private List createServices() { }); managerFuture.thenApply(manager -> { - for (AbstractStorageEngine storageEngine: storageService.getStorageEngineRepository() - .getAllLocalStorageEngines()) { - storageEngine.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager, veniceConfigLoader); - } + storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); return true; }); From c63d62fcc15d530b352e90029c054b5babe55d25 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Thu, 3 Oct 2024 11:49:37 -0700 Subject: [PATCH 20/48] Retain relevant/used code changes --- .../com/linkedin/davinci/helix/HelixParticipationService.java | 4 ---- .../venice/listener/ServerReadMetadataRepository.java | 1 - 2 files changed, 5 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java index 07d6801de0..a0bbd04a9e 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java @@ -427,10 +427,6 @@ public Instance getInstance() { return instance; } - public SafeHelixManager getHelixManager() { - return helixManager; - } - public VeniceOfflinePushMonitorAccessor getVeniceOfflinePushMonitorAccessor() { return veniceOfflinePushMonitorAccessor; } diff --git a/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java b/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java index 50950a4531..a6126fdd43 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/listener/ServerReadMetadataRepository.java @@ -59,7 +59,6 @@ public ServerReadMetadataRepository( customizedViewFuture.ifPresent(future -> future.thenApply(cv -> this.customizedViewRepository = cv)); helixInstanceFuture.ifPresent(future -> future.thenApply(helix -> this.helixInstanceConfigRepository = helix)); - } /** From 3979b634b126fb351e1f4c081b8428e1aa8735a6 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Sun, 6 Oct 2024 22:35:34 -0700 Subject: [PATCH 21/48] StorageService unit test --- .../davinci/storage/StorageService.java | 13 ++- .../davinci/storage/StorageServiceTest.java | 93 ++++++++++++++++++- 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 1efd1bd428..c36cf4f846 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -17,6 +17,7 @@ import com.linkedin.venice.ConfigKeys; import com.linkedin.venice.exceptions.VeniceException; import com.linkedin.venice.exceptions.VeniceNoStoreException; +import com.linkedin.venice.helix.SafeHelixDataAccessor; import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; @@ -375,12 +376,20 @@ public synchronized AbstractStorageEngine openStore( } public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHelixManager manager) { + if (getStorageEngineRepository() == null || manager.getOriginalManager() == null) { + return; + } for (AbstractStorageEngine storageEngine: getStorageEngineRepository().getAllLocalStorageEngines()) { - String storageEngineName = storageEngine.toString(); + String storageEngineName = storageEngine.getStoreVersionName(); String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); PropertyKey.Builder propertyKeyBuilder = new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); - IdealState idealState = manager.getHelixDataAccessor().getProperty(propertyKeyBuilder.idealStates(storeName)); + SafeHelixDataAccessor helixDataAccessor = manager.getHelixDataAccessor(); + IdealState idealState = helixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); + + if (idealState == null) { + return; + } Set idealStatePartitionIds = new HashSet<>(); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index a86a4879be..a5478e38dc 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -1,11 +1,9 @@ package com.linkedin.davinci.storage; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +import com.linkedin.davinci.config.VeniceClusterConfig; import com.linkedin.davinci.config.VeniceConfigLoader; import com.linkedin.davinci.config.VeniceServerConfig; import com.linkedin.davinci.config.VeniceStoreVersionConfig; @@ -14,6 +12,8 @@ import com.linkedin.davinci.store.AbstractStorageEngine; import com.linkedin.davinci.store.StorageEngineFactory; import com.linkedin.venice.exceptions.VeniceNoStoreException; +import com.linkedin.venice.helix.SafeHelixDataAccessor; +import com.linkedin.venice.helix.SafeHelixManager; import com.linkedin.venice.kafka.protocol.state.PartitionState; import com.linkedin.venice.kafka.protocol.state.StoreVersionState; import com.linkedin.venice.meta.PartitionerConfig; @@ -29,6 +29,9 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import org.apache.helix.HelixManager; +import org.apache.helix.PropertyKey; +import org.apache.helix.model.IdealState; import org.mockito.internal.util.collections.Sets; import org.testng.Assert; import org.testng.annotations.Test; @@ -121,4 +124,86 @@ public void testGetStoreAndUserPartitionsMapping() { expectedMapping.put(resourceName, partitionSet); Assert.assertEquals(storageService.getStoreAndUserPartitionsMapping(), expectedMapping); } + + @Test + public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() { + VeniceConfigLoader configLoader = mock(VeniceConfigLoader.class); + VeniceServerConfig mockServerConfig = mock(VeniceServerConfig.class); + when(mockServerConfig.getDataBasePath()).thenReturn("/tmp"); + when(configLoader.getVeniceServerConfig()).thenReturn(mockServerConfig); + VeniceClusterConfig mockClusterConfig = mock(VeniceClusterConfig.class); + when(configLoader.getVeniceClusterConfig()).thenReturn(mockClusterConfig); + + AggVersionedStorageEngineStats storageEngineStats = mock(AggVersionedStorageEngineStats.class); + RocksDBMemoryStats rocksDBMemoryStats = mock(RocksDBMemoryStats.class); + InternalAvroSpecificSerializer storeVersionStateSerializer = + mock(InternalAvroSpecificSerializer.class); + InternalAvroSpecificSerializer partitionStateSerializer = + mock(InternalAvroSpecificSerializer.class); + ReadOnlyStoreRepository storeRepository = mock(ReadOnlyStoreRepository.class); + StorageEngineFactory mockStorageEngineFactory = mock(StorageEngineFactory.class); + + String resourceName = "test_store_v1"; + String storeName = "test_store"; + Store mockStore = mock(Store.class); + Version mockVersion = mock(Version.class); + PartitionerConfig mockPartitionerConfig = mock(PartitionerConfig.class); + when(mockPartitionerConfig.getAmplificationFactor()).thenReturn(1); + when(mockVersion.getPartitionerConfig()).thenReturn(mockPartitionerConfig); + when(mockStore.getVersion(1)).thenReturn(mockVersion); + + when(storeRepository.getStore(storeName)).thenReturn(mockStore); + + VeniceStoreVersionConfig storeVersionConfig = mock(VeniceStoreVersionConfig.class); + when(storeVersionConfig.getStoreVersionName()).thenReturn(resourceName); + when(storeVersionConfig.isStorePersistenceTypeKnown()).thenReturn(true); + when(storeVersionConfig.getPersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); + when(storeVersionConfig.getStorePersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); + + when(configLoader.getStoreConfig(eq(resourceName), eq(PersistenceType.BLACK_HOLE))).thenReturn(storeVersionConfig); + + AbstractStorageEngine mockStorageEngine = mock(AbstractStorageEngine.class); + when(mockStorageEngineFactory.getStorageEngine(storeVersionConfig, false)).thenReturn(mockStorageEngine); + Set partitionSet = new HashSet<>(Arrays.asList(1, 2, 3)); + when(mockStorageEngine.getPersistedPartitionIds()).thenReturn(partitionSet); + when(mockStorageEngine.getStoreVersionName()).thenReturn(resourceName); + when(mockStorageEngineFactory.getPersistedStoreNames()).thenReturn(Sets.newSet(resourceName)); + when(mockStorageEngineFactory.getPersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); + + Map persistenceTypeToStorageEngineFactoryMap = new HashMap<>(); + persistenceTypeToStorageEngineFactoryMap.put(PersistenceType.BLACK_HOLE, mockStorageEngineFactory); + StorageService storageService = new StorageService( + configLoader, + storageEngineStats, + rocksDBMemoryStats, + storeVersionStateSerializer, + partitionStateSerializer, + storeRepository, + true, + true, + (s) -> true, + Optional.of(persistenceTypeToStorageEngineFactoryMap)); + + String clusterName = "test_cluster"; + VeniceClusterConfig veniceClusterConfig = mock(VeniceClusterConfig.class); + when(configLoader.getVeniceClusterConfig()).thenReturn(veniceClusterConfig); + when(veniceClusterConfig.getClusterName()).thenReturn(clusterName); + + StorageService mockStorageService = mock(StorageService.class); + SafeHelixManager manager = mock(SafeHelixManager.class); + HelixManager helixManager = mock(HelixManager.class); + when(manager.getOriginalManager()).thenReturn(helixManager); + + SafeHelixDataAccessor helixDataAccessor = mock(SafeHelixDataAccessor.class); + when(manager.getHelixDataAccessor()).thenReturn(helixDataAccessor); + PropertyKey key = mock(PropertyKey.class); + IdealState idealState = mock(IdealState.class); + when(helixDataAccessor.getProperty(key)).thenReturn(idealState); + Set helixPartitionSet = new HashSet<>(); + when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); + + doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + } } From b926839ab25b0e663273e1a798ede4aa8a4c4699 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 7 Oct 2024 12:53:42 -0700 Subject: [PATCH 22/48] Updates to StorageService unit test --- .../davinci/storage/StorageService.java | 2 +- .../davinci/storage/StorageServiceTest.java | 103 +++++++----------- 2 files changed, 38 insertions(+), 67 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index c36cf4f846..20b7ebd911 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -376,7 +376,7 @@ public synchronized AbstractStorageEngine openStore( } public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHelixManager manager) { - if (getStorageEngineRepository() == null || manager.getOriginalManager() == null) { + if (getStorageEngineRepository() == null || manager == null) { return; } for (AbstractStorageEngine storageEngine: getStorageEngineRepository().getAllLocalStorageEngines()) { diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index a5478e38dc..4f4c2544ef 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -1,7 +1,12 @@ package com.linkedin.davinci.storage; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.doCallRealMethod; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.linkedin.davinci.config.VeniceClusterConfig; import com.linkedin.davinci.config.VeniceConfigLoader; @@ -23,9 +28,12 @@ import com.linkedin.venice.meta.Version; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.utils.Utils; +import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -126,84 +134,47 @@ public void testGetStoreAndUserPartitionsMapping() { } @Test - public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() { - VeniceConfigLoader configLoader = mock(VeniceConfigLoader.class); - VeniceServerConfig mockServerConfig = mock(VeniceServerConfig.class); - when(mockServerConfig.getDataBasePath()).thenReturn("/tmp"); - when(configLoader.getVeniceServerConfig()).thenReturn(mockServerConfig); - VeniceClusterConfig mockClusterConfig = mock(VeniceClusterConfig.class); - when(configLoader.getVeniceClusterConfig()).thenReturn(mockClusterConfig); - - AggVersionedStorageEngineStats storageEngineStats = mock(AggVersionedStorageEngineStats.class); - RocksDBMemoryStats rocksDBMemoryStats = mock(RocksDBMemoryStats.class); - InternalAvroSpecificSerializer storeVersionStateSerializer = - mock(InternalAvroSpecificSerializer.class); - InternalAvroSpecificSerializer partitionStateSerializer = - mock(InternalAvroSpecificSerializer.class); - ReadOnlyStoreRepository storeRepository = mock(ReadOnlyStoreRepository.class); - StorageEngineFactory mockStorageEngineFactory = mock(StorageEngineFactory.class); + public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFieldException, IllegalAccessException { + StorageService mockStorageService = mock(StorageService.class); + SafeHelixManager manager = mock(SafeHelixManager.class); + HelixManager helixManager = mock(HelixManager.class); + when(manager.getOriginalManager()).thenReturn(helixManager); + StorageEngineRepository mockStorageEngineRepository = mock(StorageEngineRepository.class); + AbstractStorageEngine abstractStorageEngine = mock(AbstractStorageEngine.class); + mockStorageEngineRepository.addLocalStorageEngine(abstractStorageEngine); - String resourceName = "test_store_v1"; String storeName = "test_store"; - Store mockStore = mock(Store.class); - Version mockVersion = mock(Version.class); - PartitionerConfig mockPartitionerConfig = mock(PartitionerConfig.class); - when(mockPartitionerConfig.getAmplificationFactor()).thenReturn(1); - when(mockVersion.getPartitionerConfig()).thenReturn(mockPartitionerConfig); - when(mockStore.getVersion(1)).thenReturn(mockVersion); - - when(storeRepository.getStore(storeName)).thenReturn(mockStore); - - VeniceStoreVersionConfig storeVersionConfig = mock(VeniceStoreVersionConfig.class); - when(storeVersionConfig.getStoreVersionName()).thenReturn(resourceName); - when(storeVersionConfig.isStorePersistenceTypeKnown()).thenReturn(true); - when(storeVersionConfig.getPersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); - when(storeVersionConfig.getStorePersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); - - when(configLoader.getStoreConfig(eq(resourceName), eq(PersistenceType.BLACK_HOLE))).thenReturn(storeVersionConfig); - - AbstractStorageEngine mockStorageEngine = mock(AbstractStorageEngine.class); - when(mockStorageEngineFactory.getStorageEngine(storeVersionConfig, false)).thenReturn(mockStorageEngine); - Set partitionSet = new HashSet<>(Arrays.asList(1, 2, 3)); - when(mockStorageEngine.getPersistedPartitionIds()).thenReturn(partitionSet); - when(mockStorageEngine.getStoreVersionName()).thenReturn(resourceName); - when(mockStorageEngineFactory.getPersistedStoreNames()).thenReturn(Sets.newSet(resourceName)); - when(mockStorageEngineFactory.getPersistenceType()).thenReturn(PersistenceType.BLACK_HOLE); - - Map persistenceTypeToStorageEngineFactoryMap = new HashMap<>(); - persistenceTypeToStorageEngineFactoryMap.put(PersistenceType.BLACK_HOLE, mockStorageEngineFactory); - StorageService storageService = new StorageService( - configLoader, - storageEngineStats, - rocksDBMemoryStats, - storeVersionStateSerializer, - partitionStateSerializer, - storeRepository, - true, - true, - (s) -> true, - Optional.of(persistenceTypeToStorageEngineFactoryMap)); + when(abstractStorageEngine.getStoreVersionName()).thenReturn(storeName); String clusterName = "test_cluster"; - VeniceClusterConfig veniceClusterConfig = mock(VeniceClusterConfig.class); - when(configLoader.getVeniceClusterConfig()).thenReturn(veniceClusterConfig); - when(veniceClusterConfig.getClusterName()).thenReturn(clusterName); + VeniceConfigLoader mockVeniceConfigLoader = mock(VeniceConfigLoader.class); + VeniceServerConfig mockServerConfig = mock(VeniceServerConfig.class); + VeniceClusterConfig mockClusterConfig = mock(VeniceClusterConfig.class); + when(mockServerConfig.getDataBasePath()).thenReturn("/tmp"); + when(mockVeniceConfigLoader.getVeniceServerConfig()).thenReturn(mockServerConfig); + when(mockVeniceConfigLoader.getVeniceClusterConfig()).thenReturn(mockClusterConfig); + when(mockVeniceConfigLoader.getVeniceClusterConfig().getClusterName()).thenReturn(clusterName); - StorageService mockStorageService = mock(StorageService.class); - SafeHelixManager manager = mock(SafeHelixManager.class); - HelixManager helixManager = mock(HelixManager.class); - when(manager.getOriginalManager()).thenReturn(helixManager); + List localStorageEngines = new ArrayList<>(); + localStorageEngines.add(abstractStorageEngine); SafeHelixDataAccessor helixDataAccessor = mock(SafeHelixDataAccessor.class); when(manager.getHelixDataAccessor()).thenReturn(helixDataAccessor); - PropertyKey key = mock(PropertyKey.class); IdealState idealState = mock(IdealState.class); - when(helixDataAccessor.getProperty(key)).thenReturn(idealState); + when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); Set helixPartitionSet = new HashSet<>(); when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); + Field storageEngineRepositoryField = StorageService.class.getDeclaredField("storageEngineRepository"); + storageEngineRepositoryField.setAccessible(true); + storageEngineRepositoryField.set(mockStorageService, mockStorageEngineRepository); + when(mockStorageService.getStorageEngineRepository()).thenReturn(mockStorageEngineRepository); + when(mockStorageService.getStorageEngineRepository().getAllLocalStorageEngines()).thenReturn(localStorageEngines); + Field configLoaderField = StorageService.class.getDeclaredField("configLoader"); + configLoaderField.setAccessible(true); + configLoaderField.set(mockStorageService, mockVeniceConfigLoader); + doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); - storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); } } From 9a8b968b502fe27d75fb6f421a11c565b9218e88 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 8 Oct 2024 14:55:09 -0700 Subject: [PATCH 23/48] StorageService + unit test update --- .../davinci/storage/StorageService.java | 30 +++++++++---------- .../davinci/store/AbstractStorageEngine.java | 4 +++ .../davinci/storage/StorageServiceTest.java | 13 ++++++-- .../linkedin/venice/server/VeniceServer.java | 18 +---------- 4 files changed, 31 insertions(+), 34 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 20b7ebd911..3e21f19e9f 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -27,6 +27,7 @@ import com.linkedin.venice.meta.Version; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.service.AbstractVeniceService; +import com.linkedin.venice.store.rocksdb.RocksDBUtils; import com.linkedin.venice.utils.ExceptionUtils; import com.linkedin.venice.utils.LatencyUtils; import com.linkedin.venice.utils.Utils; @@ -376,33 +377,32 @@ public synchronized AbstractStorageEngine openStore( } public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHelixManager manager) { - if (getStorageEngineRepository() == null || manager == null) { + if (manager == null) { return; } for (AbstractStorageEngine storageEngine: getStorageEngineRepository().getAllLocalStorageEngines()) { String storageEngineName = storageEngine.getStoreVersionName(); String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); + Set storageEnginePartitionIds = storageEngine.getPartitionIds(); PropertyKey.Builder propertyKeyBuilder = new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); SafeHelixDataAccessor helixDataAccessor = manager.getHelixDataAccessor(); IdealState idealState = helixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); if (idealState == null) { - return; - } - - Set idealStatePartitionIds = new HashSet<>(); - - idealState.getPartitionSet().stream().forEach(partitionId -> { - idealStatePartitionIds.add(Integer.parseInt(partitionId)); - }); - Set storageEnginePartitionIds = storageEngine.getPartitionIds(); - - for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - if (idealStatePartitionIds.contains(storageEnginePartitionId)) { - continue; + continue; + } else { + Set idealStatePartitionIds = new HashSet<>(); + idealState.getPartitionSet().stream().forEach(partitionDbName -> { + idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); + }); + + for (Integer storageEnginePartitionId: storageEnginePartitionIds) { + if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + continue; + } + storageEngine.dropPartition(storageEnginePartitionId); } - storageEngine.dropPartition(storageEnginePartitionId); } } } diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index 1562c71469..8745dc9283 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -695,6 +695,10 @@ public synchronized Set getPartitionIds() { return this.partitionList.values().stream().map(Partition::getPartitionId).collect(Collectors.toSet()); } + public synchronized SparseConcurrentList getPartitionList() { + return this.partitionList; + } + public AbstractStoragePartition getPartitionOrThrow(int partitionId) { AbstractStoragePartition partition; ReadWriteLock readWriteLock = getRWLockForPartitionOrThrow(partitionId); diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index 4f4c2544ef..e079ceba8d 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -143,8 +143,12 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi AbstractStorageEngine abstractStorageEngine = mock(AbstractStorageEngine.class); mockStorageEngineRepository.addLocalStorageEngine(abstractStorageEngine); + String resourceName = "test_store_v1"; String storeName = "test_store"; - when(abstractStorageEngine.getStoreVersionName()).thenReturn(storeName); + when(abstractStorageEngine.getStoreVersionName()).thenReturn(resourceName); + abstractStorageEngine.addStoragePartition(1); + abstractStorageEngine.addStoragePartition(2); + abstractStorageEngine.addStoragePartition(3); String clusterName = "test_cluster"; VeniceConfigLoader mockVeniceConfigLoader = mock(VeniceConfigLoader.class); @@ -162,8 +166,10 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(manager.getHelixDataAccessor()).thenReturn(helixDataAccessor); IdealState idealState = mock(IdealState.class); when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); - Set helixPartitionSet = new HashSet<>(); + Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_1", "test_store_v1_2")); when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); + Set partitionSet = new HashSet<>(Arrays.asList(1, 2, 3)); + when(abstractStorageEngine.getPartitionIds()).thenReturn(partitionSet); Field storageEngineRepositoryField = StorageService.class.getDeclaredField("storageEngineRepository"); storageEngineRepositoryField.setAccessible(true); @@ -173,6 +179,9 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi Field configLoaderField = StorageService.class.getDeclaredField("configLoader"); configLoaderField.setAccessible(true); configLoaderField.set(mockStorageService, mockVeniceConfigLoader); + Field partitionListField = AbstractStorageEngine.class.getDeclaredField("partitionList"); + partitionListField.setAccessible(true); + partitionListField.set(abstractStorageEngine, abstractStorageEngine.getPartitionList()); doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 3727af035b..1a8e06e644 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -42,7 +42,6 @@ import com.linkedin.venice.listener.ServerReadMetadataRepository; import com.linkedin.venice.listener.ServerStoreAclHandler; import com.linkedin.venice.listener.StoreValueSchemasCacheService; -import com.linkedin.venice.meta.IngestionMode; import com.linkedin.venice.meta.ReadOnlyLiveClusterConfigRepository; import com.linkedin.venice.meta.ReadOnlySchemaRepository; import com.linkedin.venice.meta.ReadOnlyStoreRepository; @@ -72,7 +71,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Function; import org.apache.helix.zookeeper.impl.client.ZkClient; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -312,9 +310,6 @@ private List createServices() { ? new RocksDBMemoryStats(metricsRepository, "RocksDBMemoryStats", plainTableEnabled) : null; - boolean whetherToRestoreDataPartitions = !isIsolatedIngestion() - || veniceConfigLoader.getVeniceServerConfig().freezeIngestionIfReadyToServeOrLocalDataExists(); - // Create and add StorageService. storeRepository will be populated by StorageService storageService = new StorageService( veniceConfigLoader, @@ -322,10 +317,7 @@ private List createServices() { rocksDBMemoryStats, storeVersionStateSerializer, partitionStateSerializer, - metadataRepo, - whetherToRestoreDataPartitions, - true, - functionToCheckWhetherStorageEngineShouldBeKeptOrNot()); + metadataRepo); storageEngineMetadataService = new StorageEngineMetadataService(storageService.getStorageEngineRepository(), partitionStateSerializer); services.add(storageEngineMetadataService); @@ -706,14 +698,6 @@ protected VeniceConfigLoader getConfigLoader() { return veniceConfigLoader; } - protected final boolean isIsolatedIngestion() { - return veniceConfigLoader.getVeniceServerConfig().getIngestionMode().equals(IngestionMode.ISOLATED); - } - - private Function functionToCheckWhetherStorageEngineShouldBeKeptOrNot() { - return storageEngineName -> true; - } - public MetricsRepository getMetricsRepository() { return metricsRepository; } From a32b55ca14d1df12526492ced10ff6c2d9e95fbc Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 9 Oct 2024 17:27:45 -0700 Subject: [PATCH 24/48] Update to StorageService unit test --- .../davinci/storage/StorageServiceTest.java | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index e079ceba8d..a4fdc12f69 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -1,7 +1,9 @@ package com.linkedin.davinci.storage; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -27,6 +29,7 @@ import com.linkedin.venice.meta.Store; import com.linkedin.venice.meta.Version; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; +import com.linkedin.venice.store.rocksdb.RocksDBUtils; import com.linkedin.venice.utils.Utils; import java.lang.reflect.Field; import java.util.ArrayList; @@ -41,6 +44,8 @@ import org.apache.helix.PropertyKey; import org.apache.helix.model.IdealState; import org.mockito.internal.util.collections.Sets; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.testng.Assert; import org.testng.annotations.Test; @@ -145,10 +150,11 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi String resourceName = "test_store_v1"; String storeName = "test_store"; + when(abstractStorageEngine.getStoreVersionName()).thenReturn(resourceName); + abstractStorageEngine.addStoragePartition(0); abstractStorageEngine.addStoragePartition(1); abstractStorageEngine.addStoragePartition(2); - abstractStorageEngine.addStoragePartition(3); String clusterName = "test_cluster"; VeniceConfigLoader mockVeniceConfigLoader = mock(VeniceConfigLoader.class); @@ -166,10 +172,18 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(manager.getHelixDataAccessor()).thenReturn(helixDataAccessor); IdealState idealState = mock(IdealState.class); when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); - Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_1", "test_store_v1_2")); + Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_0", "test_store_v1_1")); when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); - Set partitionSet = new HashSet<>(Arrays.asList(1, 2, 3)); + Set partitionSet = new HashSet<>(Arrays.asList(0, 1, 2)); when(abstractStorageEngine.getPartitionIds()).thenReturn(partitionSet); + doAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + int partitionId = invocation.getArgument(0); + abstractStorageEngine.getPartitionIds().remove(partitionId); + return null; + } + }).when(abstractStorageEngine).dropPartition(anyInt()); Field storageEngineRepositoryField = StorageService.class.getDeclaredField("storageEngineRepository"); storageEngineRepositoryField.setAccessible(true); @@ -183,7 +197,13 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi partitionListField.setAccessible(true); partitionListField.set(abstractStorageEngine, abstractStorageEngine.getPartitionList()); + Set idealStatePartitionIds = new HashSet<>(); + idealState.getPartitionSet().stream().forEach(partitionDbName -> { + idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); + }); + doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + Assert.assertEquals(abstractStorageEngine.getPartitionIds(), idealStatePartitionIds); } } From 8b27c8deeade7fc7fb3e65b8f267f5134afa24cd Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 11 Oct 2024 16:08:34 -0700 Subject: [PATCH 25/48] Apply review comments and code addition for hostname comparison. --- .../davinci/storage/StorageService.java | 27 ++++++++++++++----- .../davinci/storage/StorageServiceTest.java | 19 ++++++++++++- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 3e21f19e9f..b043b1df64 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -388,20 +388,33 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); SafeHelixDataAccessor helixDataAccessor = manager.getHelixDataAccessor(); IdealState idealState = helixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); + String listenerHostName = manager.getInstanceName(); - if (idealState == null) { - continue; - } else { + if (idealState != null) { Set idealStatePartitionIds = new HashSet<>(); - idealState.getPartitionSet().stream().forEach(partitionDbName -> { + Set partitionSet = idealState.getPartitionSet(); + partitionSet.stream().forEach(partitionDbName -> { idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); }); - for (Integer storageEnginePartitionId: storageEnginePartitionIds) { - if (idealStatePartitionIds.contains(storageEnginePartitionId)) { + Map> recordMapFields = idealState.getRecord().getMapFields(); + for (String partitionDbName: recordMapFields.keySet()) { + boolean keepPartition = false; + int partitionId = RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName); + if (!storageEnginePartitionIds.contains(partitionId)) { continue; } - storageEngine.dropPartition(storageEnginePartitionId); + for (String hostName: recordMapFields.get(partitionDbName).keySet()) { + if (hostName.equals(listenerHostName)) { + keepPartition = true; + break; + } + } + if (!keepPartition) { + storageEngine.dropPartition(partitionId); + partitionSet.remove(partitionDbName); + recordMapFields.remove(partitionDbName); + } } } } diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index a4fdc12f69..3ac117400c 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -43,6 +43,7 @@ import org.apache.helix.HelixManager; import org.apache.helix.PropertyKey; import org.apache.helix.model.IdealState; +import org.apache.helix.zookeeper.datamodel.ZNRecord; import org.mockito.internal.util.collections.Sets; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -174,6 +175,22 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_0", "test_store_v1_1")); when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); + ZNRecord record = new ZNRecord("0"); + Map> mapFields = new HashMap<>(); + Map testPartitionZero = new HashMap<>(); + Map testPartitionOne = new HashMap<>(); + testPartitionZero.put("lor1-app56585.prod.linkedin.com_1690", "LEADER"); + testPartitionZero.put("lor1-app56614.prod.linkedin.com_1690", "STANDBY"); + testPartitionZero.put("lor1-app110448.prod.linkedin.com_1690", "STANDBY"); + testPartitionOne.put("lor1-app56586.prod.linkedin.com_1690", "LEADER"); + testPartitionOne.put("lor1-app71895.prod.linkedin.com_1690", "STANDBY"); + testPartitionOne.put("lor1-app111181.prod.linkedin.com_1690", "STANDBY"); + mapFields.put("test_store_v1_0", testPartitionZero); + mapFields.put("test_store_v1_1", testPartitionOne); + record.setMapFields(mapFields); + when(idealState.getRecord()).thenReturn(record); + when(manager.getInstanceName()).thenReturn("lor1-app56586.prod.linkedin.com_1690"); + Set partitionSet = new HashSet<>(Arrays.asList(0, 1, 2)); when(abstractStorageEngine.getPartitionIds()).thenReturn(partitionSet); doAnswer(new Answer() { @@ -204,6 +221,6 @@ public Object answer(InvocationOnMock invocation) throws Throwable { doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); - Assert.assertEquals(abstractStorageEngine.getPartitionIds(), idealStatePartitionIds); + Assert.assertFalse(idealState.getRecord().getMapFields().containsKey("test_store_v1_0")); } } From 3431081cd5aa84652364b1dad11a57e9518daed9 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 11 Oct 2024 17:35:23 -0700 Subject: [PATCH 26/48] Apply review comments --- .../linkedin/davinci/storage/StorageService.java | 9 ++++----- .../davinci/storage/StorageServiceTest.java | 13 +++---------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index b043b1df64..5ef90d1c61 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -397,14 +397,15 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); }); - Map> recordMapFields = idealState.getRecord().getMapFields(); - for (String partitionDbName: recordMapFields.keySet()) { + Map> mapFields = idealState.getRecord().getMapFields(); + for (Map.Entry> entry: mapFields.entrySet()) { boolean keepPartition = false; + String partitionDbName = entry.getKey(); int partitionId = RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName); if (!storageEnginePartitionIds.contains(partitionId)) { continue; } - for (String hostName: recordMapFields.get(partitionDbName).keySet()) { + for (String hostName: entry.getValue().keySet()) { if (hostName.equals(listenerHostName)) { keepPartition = true; break; @@ -412,8 +413,6 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli } if (!keepPartition) { storageEngine.dropPartition(partitionId); - partitionSet.remove(partitionDbName); - recordMapFields.remove(partitionDbName); } } } diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index 3ac117400c..525f3a88d7 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -29,7 +29,6 @@ import com.linkedin.venice.meta.Store; import com.linkedin.venice.meta.Version; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; -import com.linkedin.venice.store.rocksdb.RocksDBUtils; import com.linkedin.venice.utils.Utils; import java.lang.reflect.Field; import java.util.ArrayList; @@ -155,7 +154,6 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(abstractStorageEngine.getStoreVersionName()).thenReturn(resourceName); abstractStorageEngine.addStoragePartition(0); abstractStorageEngine.addStoragePartition(1); - abstractStorageEngine.addStoragePartition(2); String clusterName = "test_cluster"; VeniceConfigLoader mockVeniceConfigLoader = mock(VeniceConfigLoader.class); @@ -175,7 +173,7 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_0", "test_store_v1_1")); when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); - ZNRecord record = new ZNRecord("0"); + ZNRecord record = new ZNRecord("testId"); Map> mapFields = new HashMap<>(); Map testPartitionZero = new HashMap<>(); Map testPartitionOne = new HashMap<>(); @@ -191,7 +189,7 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(idealState.getRecord()).thenReturn(record); when(manager.getInstanceName()).thenReturn("lor1-app56586.prod.linkedin.com_1690"); - Set partitionSet = new HashSet<>(Arrays.asList(0, 1, 2)); + Set partitionSet = new HashSet<>(Arrays.asList(0, 1)); when(abstractStorageEngine.getPartitionIds()).thenReturn(partitionSet); doAnswer(new Answer() { @Override @@ -214,13 +212,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable { partitionListField.setAccessible(true); partitionListField.set(abstractStorageEngine, abstractStorageEngine.getPartitionList()); - Set idealStatePartitionIds = new HashSet<>(); - idealState.getPartitionSet().stream().forEach(partitionDbName -> { - idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); - }); - doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); - Assert.assertFalse(idealState.getRecord().getMapFields().containsKey("test_store_v1_0")); + Assert.assertFalse(abstractStorageEngine.containsPartition(0)); } } From 528f7a98656e336498b218445e626b23a34159ee Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 14 Oct 2024 10:46:01 -0700 Subject: [PATCH 27/48] Apply review comments --- .../linkedin/davinci/storage/StorageService.java | 11 ++--------- .../davinci/storage/StorageServiceTest.java | 15 ++++++++------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 5ef90d1c61..3fb4f74530 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -388,7 +388,7 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); SafeHelixDataAccessor helixDataAccessor = manager.getHelixDataAccessor(); IdealState idealState = helixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); - String listenerHostName = manager.getInstanceName(); + String instanceHostName = manager.getInstanceName(); if (idealState != null) { Set idealStatePartitionIds = new HashSet<>(); @@ -399,19 +399,12 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli Map> mapFields = idealState.getRecord().getMapFields(); for (Map.Entry> entry: mapFields.entrySet()) { - boolean keepPartition = false; String partitionDbName = entry.getKey(); int partitionId = RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName); if (!storageEnginePartitionIds.contains(partitionId)) { continue; } - for (String hostName: entry.getValue().keySet()) { - if (hostName.equals(listenerHostName)) { - keepPartition = true; - break; - } - } - if (!keepPartition) { + if (!entry.getValue().containsKey(instanceHostName)) { storageEngine.dropPartition(partitionId); } } diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index 525f3a88d7..b6b3906123 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -177,17 +177,17 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi Map> mapFields = new HashMap<>(); Map testPartitionZero = new HashMap<>(); Map testPartitionOne = new HashMap<>(); - testPartitionZero.put("lor1-app56585.prod.linkedin.com_1690", "LEADER"); - testPartitionZero.put("lor1-app56614.prod.linkedin.com_1690", "STANDBY"); - testPartitionZero.put("lor1-app110448.prod.linkedin.com_1690", "STANDBY"); - testPartitionOne.put("lor1-app56586.prod.linkedin.com_1690", "LEADER"); - testPartitionOne.put("lor1-app71895.prod.linkedin.com_1690", "STANDBY"); - testPartitionOne.put("lor1-app111181.prod.linkedin.com_1690", "STANDBY"); + testPartitionZero.put("host_1430", "LEADER"); + testPartitionZero.put("host_1435", "STANDBY"); + testPartitionZero.put("host_1440", "STANDBY"); + testPartitionOne.put("host_1520", "LEADER"); + testPartitionOne.put("host_1525", "STANDBY"); + testPartitionOne.put("host_1530", "STANDBY"); mapFields.put("test_store_v1_0", testPartitionZero); mapFields.put("test_store_v1_1", testPartitionOne); record.setMapFields(mapFields); when(idealState.getRecord()).thenReturn(record); - when(manager.getInstanceName()).thenReturn("lor1-app56586.prod.linkedin.com_1690"); + when(manager.getInstanceName()).thenReturn("host_1520"); Set partitionSet = new HashSet<>(Arrays.asList(0, 1)); when(abstractStorageEngine.getPartitionIds()).thenReturn(partitionSet); @@ -214,6 +214,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable { doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + verify(abstractStorageEngine).dropPartition(0); Assert.assertFalse(abstractStorageEngine.containsPartition(0)); } } From cdf99f71bb1179cbd8873222939d143a27f9bc25 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 18 Oct 2024 15:33:17 -0700 Subject: [PATCH 28/48] [server] Remove storage partitions not assigned to host This involves: 1. Updating a function in StorageService to consult ideal state 2. Hostname comparison for each partition 3. Unit test --- .../davinci/storage/StorageService.java | 25 ++++++------------- .../davinci/storage/StorageServiceTest.java | 13 +--------- 2 files changed, 8 insertions(+), 30 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 3fb4f74530..968d79041a 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -27,7 +27,6 @@ import com.linkedin.venice.meta.Version; import com.linkedin.venice.serialization.avro.InternalAvroSpecificSerializer; import com.linkedin.venice.service.AbstractVeniceService; -import com.linkedin.venice.store.rocksdb.RocksDBUtils; import com.linkedin.venice.utils.ExceptionUtils; import com.linkedin.venice.utils.LatencyUtils; import com.linkedin.venice.utils.Utils; @@ -381,30 +380,20 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli return; } for (AbstractStorageEngine storageEngine: getStorageEngineRepository().getAllLocalStorageEngines()) { - String storageEngineName = storageEngine.getStoreVersionName(); - String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); - Set storageEnginePartitionIds = storageEngine.getPartitionIds(); + String storeName = storageEngine.getStoreVersionName(); + Set storageEnginePartitionIds = new HashSet<>(storageEngine.getPartitionIds()); + String instanceHostName = manager.getInstanceName(); PropertyKey.Builder propertyKeyBuilder = new PropertyKey.Builder(configLoader.getVeniceClusterConfig().getClusterName()); SafeHelixDataAccessor helixDataAccessor = manager.getHelixDataAccessor(); IdealState idealState = helixDataAccessor.getProperty(propertyKeyBuilder.idealStates(storeName)); - String instanceHostName = manager.getInstanceName(); if (idealState != null) { - Set idealStatePartitionIds = new HashSet<>(); - Set partitionSet = idealState.getPartitionSet(); - partitionSet.stream().forEach(partitionDbName -> { - idealStatePartitionIds.add(RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName)); - }); - Map> mapFields = idealState.getRecord().getMapFields(); - for (Map.Entry> entry: mapFields.entrySet()) { - String partitionDbName = entry.getKey(); - int partitionId = RocksDBUtils.parsePartitionIdFromPartitionDbName(partitionDbName); - if (!storageEnginePartitionIds.contains(partitionId)) { - continue; - } - if (!entry.getValue().containsKey(instanceHostName)) { + for (Integer partitionId: storageEnginePartitionIds) { + String partitionDbName = storeName + "_" + partitionId; + if (!mapFields.containsKey(partitionDbName) + || !mapFields.get(partitionDbName).containsKey(instanceHostName)) { storageEngine.dropPartition(partitionId); } } diff --git a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java index b6b3906123..6f163afff5 100644 --- a/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java +++ b/clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java @@ -39,7 +39,6 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import org.apache.helix.HelixManager; import org.apache.helix.PropertyKey; import org.apache.helix.model.IdealState; import org.apache.helix.zookeeper.datamodel.ZNRecord; @@ -142,8 +141,6 @@ public void testGetStoreAndUserPartitionsMapping() { public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFieldException, IllegalAccessException { StorageService mockStorageService = mock(StorageService.class); SafeHelixManager manager = mock(SafeHelixManager.class); - HelixManager helixManager = mock(HelixManager.class); - when(manager.getOriginalManager()).thenReturn(helixManager); StorageEngineRepository mockStorageEngineRepository = mock(StorageEngineRepository.class); AbstractStorageEngine abstractStorageEngine = mock(AbstractStorageEngine.class); mockStorageEngineRepository.addLocalStorageEngine(abstractStorageEngine); @@ -157,10 +154,7 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi String clusterName = "test_cluster"; VeniceConfigLoader mockVeniceConfigLoader = mock(VeniceConfigLoader.class); - VeniceServerConfig mockServerConfig = mock(VeniceServerConfig.class); VeniceClusterConfig mockClusterConfig = mock(VeniceClusterConfig.class); - when(mockServerConfig.getDataBasePath()).thenReturn("/tmp"); - when(mockVeniceConfigLoader.getVeniceServerConfig()).thenReturn(mockServerConfig); when(mockVeniceConfigLoader.getVeniceClusterConfig()).thenReturn(mockClusterConfig); when(mockVeniceConfigLoader.getVeniceClusterConfig().getClusterName()).thenReturn(clusterName); @@ -171,8 +165,6 @@ public void testCheckWhetherStoragePartitionsShouldBeKeptOrNot() throws NoSuchFi when(manager.getHelixDataAccessor()).thenReturn(helixDataAccessor); IdealState idealState = mock(IdealState.class); when(helixDataAccessor.getProperty((PropertyKey) any())).thenReturn(idealState); - Set helixPartitionSet = new HashSet<>(Arrays.asList("test_store_v1_0", "test_store_v1_1")); - when(idealState.getPartitionSet()).thenReturn(helixPartitionSet); ZNRecord record = new ZNRecord("testId"); Map> mapFields = new HashMap<>(); Map testPartitionZero = new HashMap<>(); @@ -208,13 +200,10 @@ public Object answer(InvocationOnMock invocation) throws Throwable { Field configLoaderField = StorageService.class.getDeclaredField("configLoader"); configLoaderField.setAccessible(true); configLoaderField.set(mockStorageService, mockVeniceConfigLoader); - Field partitionListField = AbstractStorageEngine.class.getDeclaredField("partitionList"); - partitionListField.setAccessible(true); - partitionListField.set(abstractStorageEngine, abstractStorageEngine.getPartitionList()); doCallRealMethod().when(mockStorageService).checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); mockStorageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); verify(abstractStorageEngine).dropPartition(0); - Assert.assertFalse(abstractStorageEngine.containsPartition(0)); + Assert.assertFalse(abstractStorageEngine.getPartitionIds().contains(0)); } } From 4ba8f75620a1d21e470e2b96c8256a08fb7c5885 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 21 Oct 2024 09:58:06 -0700 Subject: [PATCH 29/48] [server] Drop unassigned partitions for store upon VeniceServer shutdown --- .../linkedin/davinci/helix/HelixParticipationService.java | 4 ++++ .../java/com/linkedin/davinci/storage/StorageService.java | 3 +++ .../main/java/com/linkedin/venice/server/VeniceServer.java | 7 ++----- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java index a0bbd04a9e..07d6801de0 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java @@ -427,6 +427,10 @@ public Instance getInstance() { return instance; } + public SafeHelixManager getHelixManager() { + return helixManager; + } + public VeniceOfflinePushMonitorAccessor getVeniceOfflinePushMonitorAccessor() { return veniceOfflinePushMonitorAccessor; } diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 968d79041a..0564d09c70 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -391,6 +391,9 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli if (idealState != null) { Map> mapFields = idealState.getRecord().getMapFields(); for (Integer partitionId: storageEnginePartitionIds) { + if (storageEngine.isMetadataPartition(partitionId)) { + continue; + } String partitionDbName = storeName + "_" + partitionId; if (!mapFields.containsKey(partitionDbName) || !mapFields.get(partitionDbName).containsKey(instanceHostName)) { diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 1a8e06e644..b41c4b76b5 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -355,11 +355,6 @@ private List createServices() { return helixData; }); - managerFuture.thenApply(manager -> { - storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); - return true; - }); - heartbeatMonitoringService = new HeartbeatMonitoringService( metricsRepository, metadataRepo, @@ -626,6 +621,8 @@ public void shutdown() throws VeniceException { } } + storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(helixParticipationService.getHelixManager()); + for (AbstractVeniceService service: CollectionUtils.reversed(services.get())) { try { LOGGER.info("Stopping service: {}", service.getName()); From f8c1365074cc16b6d34d22e72619ed89c6f7f52a Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 29 Oct 2024 17:12:08 -0700 Subject: [PATCH 30/48] [server] Drop unassigned partitions Unassigned partitions for store are dropped upon VeniceServer shutdown Remove storage partitions not assigned to host This involves updates in: 1. Updating a function in StorageService to consult ideal state 2. Hostname comparison for each partition 3. Resolution of empty assignment in ideal state 4. Unit test --- .../java/com/linkedin/venice/controller/VeniceHelixAdmin.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java index fe8170c501..2ef2b264a9 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java @@ -6293,6 +6293,8 @@ private void createClusterIfRequired(String clusterName) { helixClusterProperties .put(ClusterConfig.ClusterConfigProperty.DELAY_REBALANCE_TIME.name(), String.valueOf(delayedTime)); } + helixClusterProperties + .put(ClusterConfig.ClusterConfigProperty.PERSIST_BEST_POSSIBLE_ASSIGNMENT.name(), String.valueOf(true)); // Topology and fault zone type fields are used by CRUSH alg. Helix would apply the constrains on CRUSH alg to // choose proper instance to hold the replica. helixClusterProperties From 48067cc4f4f9ae68daebb21858b7027d40f8dbdf Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Thu, 31 Oct 2024 11:53:52 -0700 Subject: [PATCH 31/48] Integration Test [In Writing] --- .../venice/server/VeniceServerTest.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index 2e978c5087..c8ecdc03b1 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -9,6 +9,7 @@ import com.linkedin.d2.balancer.D2Client; import com.linkedin.davinci.listener.response.ServerCurrentVersionResponse; import com.linkedin.davinci.storage.StorageEngineRepository; +import com.linkedin.davinci.storage.StorageService; import com.linkedin.r2.message.rest.RestRequest; import com.linkedin.r2.message.rest.RestRequestBuilder; import com.linkedin.r2.message.rest.RestResponse; @@ -182,6 +183,54 @@ public void testCheckBeforeJointClusterBeforeHelixInitializingCluster() throws E } } + @Test + public void testShutdown() { + try (VeniceClusterWrapper cluster = ServiceFactory.getVeniceCluster(1, 1, 0)) { + VeniceServerWrapper server = cluster.getVeniceServers().get(0); + StorageService storageService = server.getVeniceServer().getStorageService(); + StorageEngineRepository repository = storageService.getStorageEngineRepository(); + String instanceName = server.getHost() + "_" + server.getPort(); + Assert + .assertTrue(repository.getAllLocalStorageEngines().isEmpty(), "New node should not have any storage engine."); + + // Create a storage engine. + String storeName = Version.composeKafkaTopic(Utils.getUniqueString("test_store"), 1); + storageService.openStoreForNewPartition( + server.getVeniceServer().getConfigLoader().getStoreConfig(storeName), + 1, + () -> null); + Assert.assertEquals( + repository.getAllLocalStorageEngines().size(), + 1, + "We have created one storage engine for store: " + storeName); + + // Restart server, as server's info leave in Helix cluster, so we expect that all local storage would NOT be + // deleted + // once the server join again. + cluster.stopVeniceServer(server.getPort()); + cluster.restartVeniceServer(server.getPort()); + repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1, "We should not cleanup the local storage"); + + // Stop server, remove it from the cluster then restart. We expect that all local storage would be deleted. Once + // the server join again. + cluster.stopVeniceServer(server.getPort()); + try (ControllerClient client = ControllerClient + .constructClusterControllerClient(cluster.getClusterName(), cluster.getAllControllersURLs())) { + client.removeNodeFromCluster(Utils.getHelixNodeIdentifier(Utils.getHostName(), server.getPort())); + } + + cluster.restartVeniceServer(server.getPort()); + Assert.assertTrue( + server.getVeniceServer() + .getStorageService() + .getStorageEngineRepository() + .getAllLocalStorageEngines() + .isEmpty(), + "After removing the node from cluster, local storage should be cleaned up once the server join the cluster again."); + } + } + @Test public void testMetadataFetchRequest() throws ExecutionException, InterruptedException, IOException { Utils.thisIsLocalhost(); From d6f835eb1905ae73496e5683f53f39f3a304a303 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 1 Nov 2024 15:57:52 -0700 Subject: [PATCH 32/48] Integration Test [In Writing] --- .../com/linkedin/davinci/store/AbstractStorageEngine.java | 4 ---- .../java/com/linkedin/venice/server/VeniceServerTest.java | 4 +++- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java index 8745dc9283..1562c71469 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/store/AbstractStorageEngine.java @@ -695,10 +695,6 @@ public synchronized Set getPartitionIds() { return this.partitionList.values().stream().map(Partition::getPartitionId).collect(Collectors.toSet()); } - public synchronized SparseConcurrentList getPartitionList() { - return this.partitionList; - } - public AbstractStoragePartition getPartitionOrThrow(int partitionId) { AbstractStoragePartition partition; ReadWriteLock readWriteLock = getRWLockForPartitionOrThrow(partitionId); diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index c8ecdc03b1..069cca4232 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -189,7 +189,6 @@ public void testShutdown() { VeniceServerWrapper server = cluster.getVeniceServers().get(0); StorageService storageService = server.getVeniceServer().getStorageService(); StorageEngineRepository repository = storageService.getStorageEngineRepository(); - String instanceName = server.getHost() + "_" + server.getPort(); Assert .assertTrue(repository.getAllLocalStorageEngines().isEmpty(), "New node should not have any storage engine."); @@ -203,6 +202,9 @@ public void testShutdown() { repository.getAllLocalStorageEngines().size(), 1, "We have created one storage engine for store: " + storeName); + Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 1); + + server.getVeniceServer().shutdown(); // Restart server, as server's info leave in Helix cluster, so we expect that all local storage would NOT be // deleted From 0730b55006063f43fa58f157f8b3f9afb2299377 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:45:16 -0800 Subject: [PATCH 33/48] [server] Remove partitions not assigned to current host. Wrote related function, unit test, integration test --- .../venice/server/VeniceServerTest.java | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index 069cca4232..3586f77927 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -193,7 +193,7 @@ public void testShutdown() { .assertTrue(repository.getAllLocalStorageEngines().isEmpty(), "New node should not have any storage engine."); // Create a storage engine. - String storeName = Version.composeKafkaTopic(Utils.getUniqueString("test_store"), 1); + String storeName = Version.composeKafkaTopic(cluster.createStore(1), 1); storageService.openStoreForNewPartition( server.getVeniceServer().getConfigLoader().getStoreConfig(storeName), 1, @@ -202,34 +202,19 @@ public void testShutdown() { repository.getAllLocalStorageEngines().size(), 1, "We have created one storage engine for store: " + storeName); - Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 1); + Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); server.getVeniceServer().shutdown(); + Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); + // Restart server, as server's info leave in Helix cluster, so we expect that all local storage would NOT be // deleted // once the server join again. cluster.stopVeniceServer(server.getPort()); cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); - Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1, "We should not cleanup the local storage"); - - // Stop server, remove it from the cluster then restart. We expect that all local storage would be deleted. Once - // the server join again. - cluster.stopVeniceServer(server.getPort()); - try (ControllerClient client = ControllerClient - .constructClusterControllerClient(cluster.getClusterName(), cluster.getAllControllersURLs())) { - client.removeNodeFromCluster(Utils.getHelixNodeIdentifier(Utils.getHostName(), server.getPort())); - } - - cluster.restartVeniceServer(server.getPort()); - Assert.assertTrue( - server.getVeniceServer() - .getStorageService() - .getStorageEngineRepository() - .getAllLocalStorageEngines() - .isEmpty(), - "After removing the node from cluster, local storage should be cleaned up once the server join the cluster again."); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1, "Storage engine should remain."); } } From 850f586c46cb0fd15f0bed585b7dc0b12bf0abba Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Mon, 4 Nov 2024 16:56:37 -0800 Subject: [PATCH 34/48] Simplified Integration Test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index 3586f77927..fc914ed488 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -207,14 +207,6 @@ public void testShutdown() { server.getVeniceServer().shutdown(); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); - - // Restart server, as server's info leave in Helix cluster, so we expect that all local storage would NOT be - // deleted - // once the server join again. - cluster.stopVeniceServer(server.getPort()); - cluster.restartVeniceServer(server.getPort()); - repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); - Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1, "Storage engine should remain."); } } From 4547ce7d2b2aee70be4e10504465667c7a6bf74e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 8 Nov 2024 16:50:28 -0800 Subject: [PATCH 35/48] Apply review comments [server] Remove storage partitions not assigned to host. Drop unassigned partitions. Update to server to verify partitions to persist. This involves: 1. Updating a function in StorageService to consult ideal state 2. Hostname comparison for each partition 3. Correct call in VeniceServer 4. Unit test 5. Integration test --- .../helix/HelixParticipationService.java | 4 ---- .../davinci/storage/StorageService.java | 3 --- .../venice/server/VeniceServerTest.java | 23 ++++++++----------- .../linkedin/venice/server/VeniceServer.java | 7 ++++-- 4 files changed, 14 insertions(+), 23 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java index 119cc2258b..12d93b559d 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/helix/HelixParticipationService.java @@ -452,10 +452,6 @@ public Instance getInstance() { return instance; } - public SafeHelixManager getHelixManager() { - return helixManager; - } - public VeniceOfflinePushMonitorAccessor getVeniceOfflinePushMonitorAccessor() { return veniceOfflinePushMonitorAccessor; } diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 0564d09c70..968d79041a 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -391,9 +391,6 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli if (idealState != null) { Map> mapFields = idealState.getRecord().getMapFields(); for (Integer partitionId: storageEnginePartitionIds) { - if (storageEngine.isMetadataPartition(partitionId)) { - continue; - } String partitionDbName = storeName + "_" + partitionId; if (!mapFields.containsKey(partitionDbName) || !mapFields.get(partitionDbName).containsKey(instanceHostName)) { diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index fc914ed488..bdedca8410 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -184,9 +184,14 @@ public void testCheckBeforeJointClusterBeforeHelixInitializingCluster() throws E } @Test - public void testShutdown() { - try (VeniceClusterWrapper cluster = ServiceFactory.getVeniceCluster(1, 1, 0)) { + public void testStartServer() { + try (VeniceClusterWrapper cluster = ServiceFactory.getVeniceCluster(1, 0, 0)) { + Properties featureProperties = new Properties(); + featureProperties.setProperty(SERVER_ENABLE_SERVER_ALLOW_LIST, Boolean.toString(true)); + featureProperties.setProperty(SERVER_IS_AUTO_JOIN, Boolean.toString(true)); + cluster.addVeniceServer(featureProperties, new Properties()); VeniceServerWrapper server = cluster.getVeniceServers().get(0); + Assert.assertTrue(server.getVeniceServer().isStarted()); StorageService storageService = server.getVeniceServer().getStorageService(); StorageEngineRepository repository = storageService.getStorageEngineRepository(); Assert @@ -194,19 +199,9 @@ public void testShutdown() { // Create a storage engine. String storeName = Version.composeKafkaTopic(cluster.createStore(1), 1); - storageService.openStoreForNewPartition( - server.getVeniceServer().getConfigLoader().getStoreConfig(storeName), - 1, - () -> null); - Assert.assertEquals( - repository.getAllLocalStorageEngines().size(), - 1, - "We have created one storage engine for store: " + storeName); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); + Assert.assertTrue(server.getVeniceServer().getHelixParticipationService().isRunning()); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); - - server.getVeniceServer().shutdown(); - - Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); } } diff --git a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java index 9adeb1a554..2d80fd0930 100644 --- a/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java +++ b/services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java @@ -355,6 +355,11 @@ private List createServices() { return helixData; }); + managerFuture.thenApply(manager -> { + storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(manager); + return true; + }); + heartbeatMonitoringService = new HeartbeatMonitoringService( metricsRepository, metadataRepo, @@ -625,8 +630,6 @@ public void shutdown() throws VeniceException { } } - storageService.checkWhetherStoragePartitionsShouldBeKeptOrNot(helixParticipationService.getHelixManager()); - for (AbstractVeniceService service: CollectionUtils.reversed(services.get())) { try { LOGGER.info("Stopping service: {}", service.getName()); From 93e907681742b5032202b9fa8956850e90140be5 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Sun, 10 Nov 2024 21:17:21 -0800 Subject: [PATCH 36/48] Modified Integration Test --- .../com/linkedin/venice/server/VeniceServerTest.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index bdedca8410..a0cecbfda9 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -184,7 +184,7 @@ public void testCheckBeforeJointClusterBeforeHelixInitializingCluster() throws E } @Test - public void testStartServer() { + public void testStartServerAndShutdownWithPartitionAssignmentVerification() { try (VeniceClusterWrapper cluster = ServiceFactory.getVeniceCluster(1, 0, 0)) { Properties featureProperties = new Properties(); featureProperties.setProperty(SERVER_ENABLE_SERVER_ALLOW_LIST, Boolean.toString(true)); @@ -202,6 +202,15 @@ public void testStartServer() { Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); Assert.assertTrue(server.getVeniceServer().getHelixParticipationService().isRunning()); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); + + server.getVeniceServer().shutdown(); + cluster.getControllerClient().deleteStore(storeName); + + cluster.stopVeniceServer(server.getPort()); + cluster.restartVeniceServer(server.getPort()); + repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); + Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); } } From 9186d1f9ce8e2987862373d964053316ed5ce16f Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 12 Nov 2024 12:14:37 -0800 Subject: [PATCH 37/48] Modified Integration Test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index a0cecbfda9..d278006dba 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -204,13 +204,14 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); server.getVeniceServer().shutdown(); - cluster.getControllerClient().deleteStore(storeName); cluster.stopVeniceServer(server.getPort()); cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); + + cluster.getControllerClient().deleteStore(storeName); } } From fa55047ebee8cf5eb6d27558272be3c39170239e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:08:29 -0800 Subject: [PATCH 38/48] Modified Integration Test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index d278006dba..d3dd4cc205 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -203,15 +203,12 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { Assert.assertTrue(server.getVeniceServer().getHelixParticipationService().isRunning()); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); - server.getVeniceServer().shutdown(); - cluster.stopVeniceServer(server.getPort()); + cluster.getControllerClient().deleteStore(storeName); cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); - - cluster.getControllerClient().deleteStore(storeName); } } From 494423c17702dd019a5609b902b54775f881400e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 13 Nov 2024 11:33:42 -0800 Subject: [PATCH 39/48] Modified Integration Test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index d3dd4cc205..5c74cdc21c 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -209,6 +209,9 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); + server.getVeniceServer().getStorageService().removeStorageEngine(storeName); + repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 0); } } From ef9e08ce299f5a4f625a88fde0fc5516e9b5a69e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 13 Nov 2024 14:37:12 -0800 Subject: [PATCH 40/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped --- .../main/java/com/linkedin/davinci/storage/StorageService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 968d79041a..85290a6241 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -397,6 +397,9 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli storageEngine.dropPartition(partitionId); } } + if (storageEngine.getPartitionIds().isEmpty()) { + removeStorageEngine(storeName); + } } } } From 9bc2b7e4203526e5af96e917b5a578ad4b9fdeee Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:56:16 -0800 Subject: [PATCH 41/48] Modified Integration Test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index 5c74cdc21c..d3dd4cc205 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -209,9 +209,6 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); - server.getVeniceServer().getStorageService().removeStorageEngine(storeName); - repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); - Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 0); } } From 8d07fb54f16a475673901bebf63763ffa23a1c6e Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Thu, 14 Nov 2024 14:06:00 -0800 Subject: [PATCH 42/48] Modified Integration Test --- .../com/linkedin/venice/server/VeniceServerTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index d3dd4cc205..b96499b250 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -204,11 +204,14 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 3); cluster.stopVeniceServer(server.getPort()); - cluster.getControllerClient().deleteStore(storeName); + + // Create new servers so partition assignment is removed for the offline participant + cluster.addVeniceServer(featureProperties, new Properties()); + cluster.addVeniceServer(featureProperties, new Properties()); + cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); - Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 1); - Assert.assertEquals(storageService.getStorageEngine(storeName).getPartitionIds().size(), 0); + Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 0); } } From cb1131f551a85f1f97b2d8377f8bf1b43c572251 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 15 Nov 2024 12:56:24 -0800 Subject: [PATCH 43/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped. Dropped metadata partition from storage engine. --- .../main/java/com/linkedin/davinci/storage/StorageService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 85290a6241..9824eeb259 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -391,6 +391,9 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli if (idealState != null) { Map> mapFields = idealState.getRecord().getMapFields(); for (Integer partitionId: storageEnginePartitionIds) { + if (storageEngine.isMetadataPartition(partitionId)) { + storageEngine.dropPartition(partitionId); + } String partitionDbName = storeName + "_" + partitionId; if (!mapFields.containsKey(partitionDbName) || !mapFields.get(partitionDbName).containsKey(instanceHostName)) { From 797f809c4b9f4d922cbf5c08529b015cf384c640 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 15 Nov 2024 14:42:21 -0800 Subject: [PATCH 44/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped. --- .../java/com/linkedin/davinci/storage/StorageService.java | 2 +- .../java/com/linkedin/venice/server/VeniceServerTest.java | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 9824eeb259..5229a7d9d6 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -392,7 +392,7 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli Map> mapFields = idealState.getRecord().getMapFields(); for (Integer partitionId: storageEnginePartitionIds) { if (storageEngine.isMetadataPartition(partitionId)) { - storageEngine.dropPartition(partitionId); + continue; } String partitionDbName = storeName + "_" + partitionId; if (!mapFields.containsKey(partitionDbName) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index b96499b250..cd774ae37c 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -205,10 +205,6 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { cluster.stopVeniceServer(server.getPort()); - // Create new servers so partition assignment is removed for the offline participant - cluster.addVeniceServer(featureProperties, new Properties()); - cluster.addVeniceServer(featureProperties, new Properties()); - cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 0); From 7d6246e549d61bbf73ed21d818fc434e5c54e1b9 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 15 Nov 2024 15:13:25 -0800 Subject: [PATCH 45/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped. Drop unassigned partitions. Update to server to verify partitions to persist. This involves: 1. Updating a function in StorageService to consult ideal state. 2. Hostname comparison for each partition. 3. Correct call in VeniceServer 4. Unit test 5. Integration test --- .../java/com/linkedin/venice/server/VeniceServerTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java index cd774ae37c..b96499b250 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java @@ -205,6 +205,10 @@ public void testStartServerAndShutdownWithPartitionAssignmentVerification() { cluster.stopVeniceServer(server.getPort()); + // Create new servers so partition assignment is removed for the offline participant + cluster.addVeniceServer(featureProperties, new Properties()); + cluster.addVeniceServer(featureProperties, new Properties()); + cluster.restartVeniceServer(server.getPort()); repository = server.getVeniceServer().getStorageService().getStorageEngineRepository(); Assert.assertEquals(repository.getAllLocalStorageEngines().size(), 0); From bbb7873b4e27615b58beabc3fdaf072892b17671 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Fri, 15 Nov 2024 21:47:10 -0800 Subject: [PATCH 46/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped and when idealState is null. --- .../main/java/com/linkedin/davinci/storage/StorageService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 5229a7d9d6..0988c4925d 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -403,6 +403,8 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli if (storageEngine.getPartitionIds().isEmpty()) { removeStorageEngine(storeName); } + } else { + removeStorageEngine(storeName); } } } From 082c11ce6a8a9e5ab81a931c708821dbacfe2f8b Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Sat, 16 Nov 2024 19:20:45 -0800 Subject: [PATCH 47/48] [server] Remove storage partitions not assigned to current host. Drop storage engine if all partitions have been dropped and when idealState is null. --- .../java/com/linkedin/davinci/storage/StorageService.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 0988c4925d..0bab363029 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -404,7 +404,9 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli removeStorageEngine(storeName); } } else { - removeStorageEngine(storeName); + if (storageEngine.getPartitionIds().isEmpty()) { + removeStorageEngine(storeName); + } } } } From f7787e633c42d2961f04db4e7b95addb72d1bc24 Mon Sep 17 00:00:00 2001 From: Kristy Lee <18691669+kristyelee@users.noreply.github.com> Date: Sun, 17 Nov 2024 15:58:52 -0800 Subject: [PATCH 48/48] Temp. code --- .../java/com/linkedin/davinci/storage/StorageService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java index 0bab363029..9c449160af 100644 --- a/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java +++ b/clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java @@ -404,9 +404,9 @@ public synchronized void checkWhetherStoragePartitionsShouldBeKeptOrNot(SafeHeli removeStorageEngine(storeName); } } else { - if (storageEngine.getPartitionIds().isEmpty()) { - removeStorageEngine(storeName); - } + // if (storageEngine.getPartitionIds().isEmpty()) { + // removeStorageEngine(storeName); + // } } } }