[server] Dropping unassigned partitions #1196
Open
+157
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kvargha
requested changes
Sep 24, 2024
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Outdated
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
kvargha
requested changes
Sep 24, 2024
internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java
Outdated
Show resolved
Hide resolved
kristyelee
force-pushed
the
kristy_lee/650
branch
from
2e40626
to
01ca67f
Compare
|
It looks like spotBugs is failing. If you run this locally, you'll be able to read the report to see what's wrong. Make sure to build before running this. |
kristyelee
force-pushed
the
kristy_lee/650
branch
2 times, most recently
from
3e496b1
to
56e7711
Compare
kristyelee
force-pushed
the
kristy_lee/650
branch
from
334c1f6
to
04c4e9e
Compare
kristyelee
commented
Sep 26, 2024
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
…ckWhetherStoragePartitionShouldBeKeptOrNot
kvargha
reviewed
Sep 27, 2024
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
kristyelee
commented
Sep 30, 2024
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
nisargthakkar
changed the title
kristyelee
force-pushed
the
kristy_lee/650
branch
from
142082c
to
4ba8f75
Compare
kvargha
reviewed
Oct 28, 2024
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
kristyelee
force-pushed
the
kristy_lee/650
branch
from
5e43f6f
to
4ba8f75
Compare
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
kristyelee
force-pushed
the
kristy_lee/650
branch
from
fa722f1
to
eb71013
Compare
kristyelee
force-pushed
the
kristy_lee/650
branch
2 times, most recently
from
8ceb7cc
to
3364f13
Compare
…d function, unit test, integration test
kristyelee
force-pushed
the
kristy_lee/650
branch
from
3364f13
to
0730b55
Compare
|
The check over which storage partitions to persist is moved to the shutdown phase of VeniceServer which was discussed. Shutdown phase contains call to |
kvargha
reviewed
Nov 8, 2024
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
...venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java
Outdated
Show resolved
Hide resolved
[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
kvargha
reviewed
Nov 12, 2024
...venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java
Show resolved
Hide resolved
...venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java
Show resolved
Hide resolved
kvargha
reviewed
Nov 12, 2024
...venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java
Outdated
Show resolved
Hide resolved
kvargha
reviewed
Nov 13, 2024
| @@ -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); | |||
There was a problem hiding this comment.
This logic should be done in checkWhetherStoragePartitionsShouldBeKeptOrNot.
If we've deleted all partitions for a storageEngine, we should call removeStorageEngine.
There was a problem hiding this comment.
Since removeStorageEngine, is now called in checkWhetherStoragePartitionsShouldBeKeptOrNot, you can remove line 212. Once that's in I'll kick it off to Zac.
… storage engine if all partitions have been dropped
kvargha
reviewed
Nov 14, 2024
Comment on lines
+391
to
+404
| if (idealState != null) { | ||
| Map<String, Map<String, String>> mapFields = idealState.getRecord().getMapFields(); | ||
| for (Integer partitionId: storageEnginePartitionIds) { | ||
| String partitionDbName = storeName + "_" + partitionId; | ||
| if (!mapFields.containsKey(partitionDbName) | ||
| || !mapFields.get(partitionDbName).containsKey(instanceHostName)) { | ||
| storageEngine.dropPartition(partitionId); | ||
| } | ||
| } | ||
| if (storageEngine.getPartitionIds().isEmpty()) { | ||
| removeStorageEngine(storeName); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
idealState can be null if the store doesn't exist anymore. I'll leave it up to @ZacAttack whether or not we should remove the storageEngine in the else conditional.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary, imperative, start upper case, don't end with a period
The goal is to remove unassigned partitions that are on disk state
This involves storage service and engine, which upon starting (constructed), will go through folders on disk and verify status of folders
Helix's ideal state is used for consulting partitions and their assignment
The change proposed includes defining a function in VeniceServer that goes through partitions in idealState and compares to partitions in storageEngine
StorageEngine object initialization is also being updated to run the new function that performs this verification.
Resolves #650
How was this PR tested?
Will write corresponding test.
EDIT: A unit test has been written.
Does this PR introduce any user-facing changes?