bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3903173357
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/snapshot/SnapshotHandler.java:
##########
@@ -172,8 +174,14 @@ public void onSnapshotLoad(final SnapshotReader reader,
long committedIndex) thr
// No need to load locally saved snapshots
if (shouldNotLoad(reader)) {
- log.info("skip to load snapshot because of should_not_load flag");
- return;
+ final File dataDir = new File(snapshotDir + File.separator +
SNAPSHOT_DATA_PATH);
Review Comment:
⚠️ Nested inside the `should_not_load` branch, this covers only the
flag-present variant of the signature #3162 records.
The issue describes the bad directory as `__raft_snapshot_meta` present,
`data/` missing, and `should_not_load` "often missing for early-return path".
With the flag absent, `shouldNotLoad(reader)` is false
(`SnapshotHandler.java:217-220`), this block is skipped, and control reaches
`businessHandler.loadSnapshot` exactly as before. That call already fails:
`RocksDBSession.loadSnapshot` throws `Snapshot file %s not exists`
(`RocksDBSession.java:740-745`), wrapped by
`BusinessHandlerImpl.java:1128-1137`. So the more common variant is unaffected,
and it fails with a RocksDB path error rather than a corruption diagnosis.
Requested change: hoist the `data/` check above the `shouldNotLoad` test and
throw with a message naming the corrupt snapshot directory, so both variants
are reported the same way.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/core/snapshot/SnapshotHandlerTest.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.store.core.snapshot;
+
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.contains;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hugegraph.store.HgStoreEngine;
+import org.apache.hugegraph.store.PartitionEngine;
+import org.apache.hugegraph.store.business.BusinessHandler;
+import org.apache.hugegraph.store.snapshot.SnapshotHandler;
+import org.apache.hugegraph.store.util.HgStoreException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import com.alipay.sofa.jraft.entity.RaftOutter;
+import com.alipay.sofa.jraft.storage.snapshot.SnapshotWriter;
+import com.google.protobuf.Message;
+
+public class SnapshotHandlerTest {
Review Comment:
⚠️ None of the new tests runs in any build, so the fix ships without an
executed guard.
`hg-store-test` runs its `src/main/java` tests through six surefire
executions with fixed include lists
(`hugegraph-store/hg-store-test/pom.xml:225-302`): `ClientSuiteTest`,
`CoreSuiteTest` plus `BatchGraphIsolationTest`, `CommonSuiteTest`,
`RocksDbSuiteTest`, `ServerSuiteTest`, `RaftSuiteTest`. `SnapshotHandlerTest`
matches none. The two new methods in `HgSnapshotHandlerTest` would only run
through `CoreSuiteTest`, whose `@RunWith`/`@Suite.SuiteClasses` block is
commented out with `HgSnapshotHandlerTest.class` inside it
(`CoreSuiteTest.java:22-44`), and the workflow never runs that profile
(`.github/workflows/pd-store-ci.yml:281-296`). Codecov agrees: 0% patch
coverage, 8 lines missing.
Requested change, both parts. This class is pure Mockito, so add it to
`RaftSuiteTest`'s `@Suite.SuiteClasses`, which `store-raftcore-test` executes.
The `HgSnapshotHandlerTest` methods need a live engine via
`StoreEngineTestBase`, so they additionally need `CoreSuiteTest` re-enabled and
a `-P store-core-test` step in the workflow.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/snapshot/SnapshotHandler.java:
##########
@@ -96,7 +96,9 @@ public void onSnapshotSave(final SnapshotWriter writer)
throws HgStoreException
Integer groupId = partitionEngine.getGroupId();
AtomicInteger state = businessHandler.getState(groupId);
if (state != null && state.get() == BusinessHandler.doing) {
- return;
+ throw new HgStoreException(
+ String.format("Partition %d is busy (compaction in
progress), " +
+ "snapshot save skipped", groupId));
Review Comment:
🧹 Three small things about this exception, assuming the throw survives the
discussion on line 99.
The text says the save was skipped, but the save now fails and reaches the
operator as a raft EIO status (`PartitionStateMachine.java:199-201`). "Skipped"
describes the behaviour this PR removes; say what happened instead, for example
`Partition %d snapshot save failed: compaction in progress`.
`new HgStoreException(String)` resolves to `EC_FAIL` (1000). The
neighbouring save failure uses a specific code, `EC_RKDB_EXPORT_SNAPSHOT_FAIL`
(`BusinessHandlerImpl.java:1123`); a dedicated code here would be easier to
grep for in the field.
Line 182 embeds a non-ASCII em dash in the new `log.warn`. At this head the
only Java files under `hugegraph-store` containing one are the three this PR
touches (`git grep -l '—' 0e1c319 -- 'hugegraph-store/*.java'`); please keep
log text ASCII.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/core/snapshot/HgSnapshotHandlerTest.java:
##########
@@ -29,8 +32,17 @@
import org.apache.hugegraph.store.core.StoreEngineTestBase;
import org.apache.hugegraph.store.meta.Partition;
import org.apache.hugegraph.store.snapshot.HgSnapshotHandler;
+import org.apache.hugegraph.store.snapshot.SnapshotHandler;
+import org.apache.hugegraph.store.util.HgStoreException;
+
+import com.alipay.sofa.jraft.entity.RaftOutter;
Review Comment:
🧹 This block duplicates the four imports already present below it.
Lines 38-41 add `RaftOutter`, `SnapshotReader`, `SnapshotWriter` and
`Message`; lines 47-50 import the same four types. Duplicate single-type
imports compile, and the checkstyle plugin is not bound for `hugegraph-store`
(only `hugegraph-server` and `hugegraph-commons` configure it), but
`style/checkstyle.xml:54` does flag `RedundantImport`. The new block also
splits the `org.junit` imports out of their group.
Requested change: drop the newly added lines 38-41 rather than the
pre-existing 47-50. That removes the duplication and restores the original
import grouping in one edit.
--
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]
