bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3941071601
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/snapshot/SnapshotHandler.java:
##########
@@ -94,35 +93,41 @@ public void onSnapshotSave(final SnapshotWriter writer)
throws HgStoreException
final String snapshotDir = writer.getPath();
if (partitionEngine != null) {
Integer groupId = partitionEngine.getGroupId();
- AtomicInteger state = businessHandler.getState(groupId);
- if (state != null && state.get() == BusinessHandler.doing) {
- return;
+ if (!businessHandler.tryLockCompactionRange(groupId)) {
+ throw new
HgStoreException(HgStoreException.EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL,
Review Comment:
‼️ This does more than refuse one snapshot: it restarts the partition's raft
node.
`PartitionStateMachine.onSnapshotSave` (line 199-201, unchanged by this PR)
turns the exception into `done.run(new Status(RaftError.EIO, ...))`, and `EIO`
is the one code jraft escalates. In `jraft-core-1.3.13`
`SnapshotExecutorImpl.onSnapshotSaveDone`, any other non-zero result only
reaches `writer.setError(...)`, but:
```java
if (ret == RaftError.EIO.getNumber()) {
reportError(RaftError.EIO.getNumber(), "Fail to save snapshot.");
}
```
`reportError` goes to `FSMCallerImpl.setError`, which calls `fsm.onError`
and `node.onError`. On this side `PartitionStateMachine.java:129-134` forwards
to the state listeners and `PartitionEngine.java:709-712` implements `onError`
as `restartRaftNode()`, i.e. `shutdown(); init(this.options);`.
`NodeImpl.onError` also steps down and sets `State.STATE_ERROR`.
So a scheduled snapshot landing inside a `compactRange()` now costs a
teardown and re-init of that partition's raft node: leader step-down,
re-election, log storage close/reopen, replay. Shipped defaults make that
overlap realistic, `snapshotInterval: 1800` in
`hg-store-node/src/main/resources/application.yml` and
`hg-store-dist/src/assembly/static/conf/application.yml`, `300` as the `@Value`
fallback in `AppConfig.java:183`, against a full RocksDB range compaction on a
large partition.
Requested change: report busy with a code jraft does not escalate,
`RaftError.EBUSY` (1009) instead of `EIO` (1014). With any non-EIO code,
`onSnapshotSaveDone` sets the writer error and `LocalSnapshotStorage.close`
destroys the temp snapshot directory without `reportError`, so the incomplete
snapshot is still refused (the #3162 fix holds) while the node keeps serving
and jraft retries next interval. That needs
`PartitionStateMachine.onSnapshotSave` to catch
`EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL` separately from a real save failure;
alternatively, wait a bounded interval for the lock here before giving up.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/snapshot/SnapshotHandler.java:
##########
@@ -169,6 +174,15 @@ private String calculateChecksum(String path) {
public void onSnapshotLoad(final SnapshotReader reader, long
committedIndex) throws
HgStoreException {
final String snapshotDir = reader.getPath();
+ final String graphSnapshotDir = snapshotDir + File.separator +
SNAPSHOT_DATA_PATH;
+
+ if (!new File(graphSnapshotDir).isDirectory()) {
Review Comment:
🧹 Placing this above the `should_not_load` early return widens it past the
corruption it targets.
Line 188 returns early for a locally saved snapshot precisely because
nothing is loaded from it. With the check above that return, such a snapshot
missing `data/` now throws, `PartitionStateMachine.onSnapshotLoad` (line
228-235) returns `false`, `FSMCallerImpl.doSnapshotLoad` calls
`setError(ESTATEMACHINE)`, and that reaches `PartitionEngine.onError` ->
`restartRaftNode()`. Re-init reads the same on-disk snapshot, so it repeats.
The #3162 signature does not need the wider placement: the old early return
in `onSnapshotSave` happened before `markShouldNotLoad`, so a snapshot
corrupted that way has no flag, `shouldNotLoad` is false, and the check still
fires from below the return.
Requested change: move the block after the `shouldNotLoad(reader)` early
return at lines 187-191.
`testOnSnapshotLoadThrowsWhenShouldNotLoadPresentButDataMissing` then needs its
expectation flipped to "skips", which is the behaviour before this PR.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/options/RaftRocksdbOptions.java:
##########
@@ -55,6 +56,15 @@ private static RocksdbConfig getRocksdbConfig(HugeConfig
options) {
}
private static void registerRaftRocksdbConfig(HugeConfig options) {
+ // StorageOptionsFactory.releaseAllOptions() (called by test setup
between runs)
+ // does not clear its table-format-config table, so registering
RocksDBLogStorage's
+ // config more than once per JVM throws IllegalStateException.
Register only once.
+ synchronized (RaftRocksdbOptions.class) {
Review Comment:
🧹 The guard releases the monitor before the registration it is guarding.
`raftRocksdbConfigRegistered` flips inside `synchronized
(RaftRocksdbOptions.class)` at line 66, but every
`StorageOptionsFactory.register*` call sits outside the block (lines 68-103,
the calls at 81, 90 and 102). Because the flag is set before the work, a throw
anywhere in the body, `new LRUCache(SizeUnit.GB)` on line 68 for instance,
permanently suppresses every later attempt in that JVM with no log line, and
the process then runs on jraft defaults instead of the configured
`DBOptions`/`ColumnFamilyOptions`. The same gap lets a concurrent second caller
return early while the first is still registering, though today only test
setups call this more than once.
Requested change: hold the monitor across the whole method body, or set
`raftRocksdbConfigRegistered = true` only after the final
`registerRocksDBColumnFamilyOptions` call.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/core/CoreSuiteTest.java:
##########
@@ -41,8 +42,10 @@
// PartitionInstructionProcessorTest.class,
// // Try to put it last
// HgBusinessImplTest.class
-//})
-
+@RunWith(Suite.class)
[email protected]({
+ HgSnapshotHandlerTest.class
Review Comment:
⚠️ Re-enabling this suite makes the `Run core test` step added in this PR
fail, and it is red on this head.
The `store-core-test` surefire execution runs `**/CoreSuiteTest.java` and
`**/BatchGraphIsolationTest.java` in one fork
(`hugegraph-store/hg-store-test/pom.xml:245-248`). With `HgSnapshotHandlerTest`
back in the suite, `CoreSuiteTest` pulls in `StoreEngineTestBase`, whose
`@AfterClass shutDownEngine()` calls `HgStoreEngine.getInstance().shutdown()`.
That singleton's closing flag is never cleared, so the next class in the same
JVM cannot open a session and `BusinessHandlerImpl.getSession` throws at
`BusinessHandlerImpl.java:1318`.
From job 101289530213, step 16:
```
10:20:50.494 [INFO] Tests run: 7 ... in
org.apache.hugegraph.store.core.CoreSuiteTest
10:20:50.505 [ERROR] HgStoreException: store is closing
at
BatchGraphIsolationTest.setup(BatchGraphIsolationTest.java:113)
[ERROR] Failed to execute goal ... maven-surefire-plugin:2.20:test
(store-core-test)
```
This is what the deleted `// TODO: uncomment it until all test can run
free.` was warning about.
Requested change: stop the two sharing a JVM. Move
`**/BatchGraphIsolationTest.java` into its own surefire execution, or set
`<reuseForks>false</reuseForks>` on `store-core-test`, so the engine
`CoreSuiteTest` shuts down cannot leak into it.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/util/HgStoreException.java:
##########
@@ -33,11 +33,9 @@ public class HgStoreException extends RuntimeException {
public static final int EC_RKDB_DOMERGE_FAIL = 1207;
public static final int EC_RKDB_DOGET_FAIL = 1208;
public static final int EC_RKDB_PD_FAIL = 1209;
- public static final int EC_RKDB_TRUNCATE_FAIL = 1212;
public static final int EC_RKDB_EXPORT_SNAPSHOT_FAIL = 1214;
public static final int EC_RKDB_IMPORT_SNAPSHOT_FAIL = 1215;
- public static final int EC_RKDB_TRANSFER_SNAPSHOT_FAIL = 1216;
- public static final int EC_METRIC_FAIL = 1401;
+ public static final int EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL = 1217;
Review Comment:
🧹 This hunk also deletes three public constants, which is unrelated to the
snapshot fix.
Alongside adding `EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL = 1217`, the diff removes
`EC_RKDB_TRUNCATE_FAIL = 1212`, `EC_RKDB_TRANSFER_SNAPSHOT_FAIL = 1216` and
`EC_METRIC_FAIL = 1401`. Grepping the head tree for the three names returns
nothing, so nothing in-repo breaks, but they are `public static final` members
of a type published in the `hg-store-core` artifact: anything downstream that
recompiles against the new jar stops compiling. Nothing in this PR needs the
removal.
Requested change: restore the three constants and keep this hunk to the
single added code. If the cleanup is wanted, it belongs in its own PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
