[ISSUE-796] Clean up PartitionType TODOs in rocksdb store module - #823
[ISSUE-796] Clean up PartitionType TODOs in rocksdb store module#823yyyCode wants to merge 2 commits into
Conversation
The `PartitionType` enum in the rocksdb store carried two stale `TODO`s: - `// TODO: Support dt partition` on `DT` — DT partition is already implemented (`SyncGraphDtPartitionProxy`, dispatched by `ProxyBuilder`), so the TODO is removed. - `// TODO: Support label dt partition` on `DT_LABEL` — this combination has no proxy implementation. Configuring it previously fell through to a generic "unexpected partition type" error. `ProxyBuilder.build` now rejects `DT_LABEL` explicitly with a message that names the unsupported type, and the TODO is replaced with a NOTE documenting the gap. Adds `PartitionTypeTest` covering the enum lookup/flags, the explicit `DT_LABEL` rejection, and that `DT` is still dispatched to a real proxy. The test targets `ProxyBuilder` directly so it does not depend on opening a native RocksDB instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| return new SyncGraphLabelPartitionProxy<>(rocksdbClient, encoder, config); | ||
| } else if (partitionType == PartitionType.DT) { | ||
| return new SyncGraphDtPartitionProxy<>(rocksdbClient, encoder, config); | ||
| } else if (partitionType == PartitionType.DT_LABEL) { |
There was a problem hiding this comment.
DT_LABEL is rejected here, but this is not fail-fast in the actual store initialization path
There was a problem hiding this comment.
Good catch. The real path is StaticGraphRocksdbStoreBase.init -> super.init() (which opens the native RocksDB in BaseRocksdbStore.init) -> ProxyBuilder.build(), so rejecting DT_LABEL only inside build() means the DB is already opened on disk before we fail.
Fixed in 5b01c37: extracted ProxyBuilder.checkPartitionTypeSupported(PartitionType) and call it in StaticGraphRocksdbStoreBase.init before super.init(), so an unsupported partition.type now fails fast without leaving a half-initialized store on disk. build() still calls the same check as well.
There was a problem hiding this comment.
Thanks for the review @Leomrlin. Verified locally on the latest commit: an unsupported partition.type now fails in StaticGraphRocksdbStoreBase.init before super.init() opens the native RocksDB, so no half-initialized store is left on disk, and ProxyBuilder.build() keeps the same guard. mvn -Dtest=PartitionTypeTest test passes 5/5. Could you take another look when you have a moment?
| * proxy constructor fails with a {@link NullPointerException}, which confirms the builder got | ||
| * past partition-type dispatch rather than throwing a {@link GeaflowRuntimeException}. | ||
| */ | ||
| @Test(expectedExceptions = NullPointerException.class) |
There was a problem hiding this comment.
NullPointerException does not clearly indicate the issue
There was a problem hiding this comment.
Agreed, inferring dispatch from an incidental NullPointerException is fragile and does not state the intent. Fixed in 5b01c37: the test now mocks RocksdbClient/encoder (mockito-all, version managed by the parent pom) and asserts the returned proxy is a SyncGraphDtPartitionProxy, so it directly verifies that DT is dispatched to the DT proxy.
There was a problem hiding this comment.
Thanks @Leomrlin. Updated so the test asserts proxy instanceof SyncGraphDtPartitionProxy directly (mocking RocksdbClient/encoder) instead of inferring dispatch from an incidental NullPointerException, which makes the intent explicit. Verified locally: PartitionTypeTest passes 5/5. PTAL when you get a chance.
- Extract ProxyBuilder.checkPartitionTypeSupported and call it in StaticGraphRocksdbStoreBase.init before super.init() opens the RocksDB instance, so an unsupported partition.type fails fast without leaving a half-initialized store on disk. - Rewrite the DT-dispatch test to mock RocksdbClient/encoder and assert the returned proxy is a SyncGraphDtPartitionProxy, instead of inferring dispatch from an incidental NullPointerException. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What changes were proposed in this pull request?
Closes #796.
Cleans up two stale
TODOs on thePartitionTypeenum in the rocksdb graph store (geaflow-store-rocksdb):DT—// TODO: Support dt partition: DT partition is already implemented viaSyncGraphDtPartitionProxyand dispatched byProxyBuilder, so this TODO is obsolete and is removed.DT_LABEL—// TODO: Support label dt partition: this combination has no proxy implementation. Previously, configuringpartition.type=dt_labelfell through to a generic"unexpected partition type"error.ProxyBuilder.buildnow rejectsDT_LABELexplicitly with a message that names the unsupported type (fail-fast), and the TODO is replaced with aNOTEdocumenting the gap for future implementers.No behavior change for the supported partition types (
NONE,LABEL,DT).How was this PR tested?
Added
PartitionTypeTestcovering:DT/DT_LABEL/NONE;ProxyBuilderrejectingDT_LABELwith an explicit message that names the type;ProxyBuilderstill dispatchingDTto a real proxy.The test targets
ProxyBuilderdirectly, so it does not require opening a native RocksDB instance.mvn test -Dtest=PartitionTypeTestpasses (5/5) andmvn checkstyle:checkreports 0 violations on the module.