-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[server] Dropping unassigned partitions #1196
base: main
Are you sure you want to change the base?
Conversation
...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
internal/venice-common/src/main/java/com/linkedin/venice/helix/SafeHelixDataAccessor.java
Outdated
Show resolved
Hide resolved
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. |
3e496b1
to
56e7711
Compare
334c1f6
to
04c4e9e
Compare
services/venice-server/src/main/java/com/linkedin/venice/server/VeniceServer.java
Outdated
Show resolved
Hide resolved
…ckWhetherStoragePartitionShouldBeKeptOrNot
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
(Suggested addition to code, that seems directly useful but is no longer / not used actively: In the |
(Reverted code in this file back to original state but thought I should make a note of this). |
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
Set<Integer> storageEnginePartitionIds = storageEngine.getPartitionIds(); | ||
|
||
for (Integer storageEnginePartitionId: storageEnginePartitionIds) { | ||
if (idealStatePartitionIds.contains(storageEnginePartitionId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also isn't quite right. The ideal state is the superset of all partitions for the store. All partitions ID's in the storageEngine will be in the superset.
We need to parse more information from the ideal state. Specifically, for each partition that is on this node, we need to determine if the ideal state for that partition id contains the node name.
To better paint the picture, an ideal state is a big json document that looks like this:
"Test_Store_Migration_Demo_v1_0": {
"lor1-app56585.prod.linkedin.com_1690": "LEADER",
"lor1-app56614.prod.linkedin.com_1690": "STANDBY",
"lor1-app110448.prod.linkedin.com_1690": "STANDBY"
},
"Test_Store_Migration_Demo_v1_1": {
"lor1-app56586.prod.linkedin.com_1690": "LEADER",
"lor1-app71895.prod.linkedin.com_1690": "STANDBY",
"lor1-app111181.prod.linkedin.com_1690": "STANDBY"
},
Here, for partitions 1 and 0, we have three hosts assigned roles for that partition. What we should do is, does the current host we're working with reside in these lists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have written the comparison and retrieved the current host from manager.getInstanceName().
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
4a4291a
to
9a8b968
Compare
f9e3464
to
a32b55c
Compare
3032ab4
to
3431081
Compare
I wrote the related unit test for this PR. |
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java
Outdated
Show resolved
Hide resolved
8385009
to
91f8519
Compare
91f8519
to
528f7a9
Compare
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java
Outdated
Show resolved
Hide resolved
6fd4c66
to
377770f
Compare
This involves: 1. Updating a function in StorageService to consult ideal state 2. Hostname comparison for each partition 3. Unit test
2ef3419
to
cdf99f7
Compare
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?