[venice-server] Dropping unassigned partitions #1196
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
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
| @@ -714,7 +714,13 @@ private Function<String, Boolean> functionToCheckWhetherStorageEngineShouldBeKep | |||
| return storageEngineName -> { | |||
| String storeName = Version.parseStoreFromKafkaTopicName(storageEngineName); | |||
|
|
|||
| AbstractStorageEngine storageEngine = storageService.getStorageEngine(storageEngineName); | |||
There was a problem hiding this comment.
Getting the correct storageEngine from storageEngineName seems to depend on consulting the directories in storageService, though when this function is called, storageService is not fully instantiated, and so the implementation in this line needs to be revised to not reference storageService.
A method that could be used: obtain the store config from veniceConfigLoader and consult factory [e.g., RocksDBStorageEngineFactory]'s getStorageEngine method. Though, the respective factory also seems to be instantiated and referenced in StorageService [in initInternalStorageEngineFactories].
There was a problem hiding this comment.
checkWhetherStorageEngineShouldBeKeptOrNot determines whether or not the entire storageEngine should be dropped, and where it's called it assumes that the storageEngine hasn't been opened yet.
What we should do here is modify the StorageService constructor to take another function that determines whether partitions should be kept or not. Ex: checkWhetherStoragePartitionsShouldBeKeptOrNot. Return type can be void to differentiate between the two checks.
This new function can be invoked here after the storageEngine has been opened.
There was a problem hiding this comment.
This idea makes sense. I will integrate this.
There was a problem hiding this comment.
Operations involving Helix [HelixParticipationService, SafeHelixDataAccessor] may require that services are started for StorageService. [requires that Venice server has started]. SafeHelixDataAccessor is used to obtain the ideal state as referenced in #650. Will have to check the structure here so that checkWhetherStoragePartitionsShouldBeKeptOrNot executes correctly.
…ckWhetherStoragePartitionShouldBeKeptOrNot
| @@ -88,6 +89,7 @@ public class StorageService extends AbstractVeniceService { | |||
| boolean restoreDataPartitions, | |||
| boolean restoreMetadataPartitions, | |||
| Function<String, Boolean> checkWhetherStorageEngineShouldBeKeptOrNot, | |||
| Function<AbstractStorageEngine, Void> checkWhetherStoragePartitionsShouldBeKeptOrNot, | |||
There was a problem hiding this comment.
Can we create an additional constructor that doesn't have checkWhetherStoragePartitionsShouldBeKeptOrNot, so if users don't want to use this, they wouldn't have to pass in a functional interface that does nothing.
There was a problem hiding this comment.
Ok, that sounds correct; checkWhetherStoragePartitionsShouldBeKeptOrNot is mainly being written in VeniceServer.
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.
Does this PR introduce any user-facing changes?