bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3885720205
##########
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(
Review Comment:
⚠️ Throwing for the whole compaction window is broader than the defect in
#3162 requires.
`BusinessHandlerImpl.dbCompaction` sets `doing` immediately before
`op.compactRange()` and clears it to `compactionDone` immediately after
(`BusinessHandlerImpl.java:1418-1421`) — a full RocksDB range compaction, which
on a large partition runs for a long time.
`PartitionStateMachine.onSnapshotSave` turns this exception into `done.run(new
Status(RaftError.EIO, e.toString()))` (`PartitionStateMachine.java:199-201`),
so with this change every jraft periodic snapshot that happens to overlap a
compaction is reported as an IO failure and the raft log is not truncated for
the duration of that compaction.
The same compaction path already arranges for a snapshot as soon as it
finishes — it logs `"dbCompaction end and start to do snapshot"` and submits
`SYNC_BLANK_TASK` (`BusinessHandlerImpl.java:1422-1437`) — which suggests the
previous early return was a deliberate skip rather than an oversight. The
defect reported in #3162 is that the skip let jraft commit the snapshot as
valid, not that the skip existed.
Requested change: bound the impact instead of failing outright — wait a
configurable interval for the state to leave `doing` (the `setAndNotifyState`
path already exists) and only throw if it is still busy when that budget
expires, so an ordinary compaction/snapshot overlap resolves itself rather than
surfacing as a raft-level IO error.
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,322 @@
+#!/usr/bin/env bash
+#
+# 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.
+
+
+# test-snapshot-corruption.sh — load-path reproducer for the HStore snapshot
corruption bug
+#
+# This script covers the LOAD side of the fix (SnapshotHandler.onSnapshotLoad):
+# default mode — simulates Sub-case A outcome (missing data/ dir) to verify
the load error
+# --fixed mode — simulates Sub-case B (should_not_load present, data/
missing) to verify Fix 2
+#
+# The SAVE side of the fix (SnapshotHandler.onSnapshotSave throwing when
compaction state==doing)
+# cannot be reproduced deterministically here: /test/compact submits a
background job and returns
+# immediately, so the race window is too narrow to hit reliably from a shell
script.
+# Save-side coverage lives in the unit test: HgSnapshotHandlerTest.
+#
+# Requires: Docker Desktop >= 20.10, >= 12 GB allocated to Docker, Docker
Compose v2
+# Run from the repo root:
+# bash docker/hbase/test/test-snapshot-corruption.sh # confirm
load-path bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm
load-path fix is active (fixed image)
Review Comment:
🧹 The documented invocation points at a path that does not exist.
Both usage lines say `bash docker/hbase/test/test-snapshot-corruption.sh`,
but the script is committed at `docker/test/test-snapshot-corruption.sh`
(confirmed against the head tree at `8e121d4d`). The code below resolves
`COMPOSE_FILE` correctly from `$SCRIPT_DIR`, so only the documentation is wrong
— but it is the first thing a reader copies, and the previous review round
already flagged the stale `docker/hbase/test` prefix.
Requested change: update both lines to `bash
docker/test/test-snapshot-corruption.sh` (and `... --fixed`).
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,322 @@
+#!/usr/bin/env bash
+#
+# 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.
+
+
+# test-snapshot-corruption.sh — load-path reproducer for the HStore snapshot
corruption bug
+#
+# This script covers the LOAD side of the fix (SnapshotHandler.onSnapshotLoad):
+# default mode — simulates Sub-case A outcome (missing data/ dir) to verify
the load error
+# --fixed mode — simulates Sub-case B (should_not_load present, data/
missing) to verify Fix 2
+#
+# The SAVE side of the fix (SnapshotHandler.onSnapshotSave throwing when
compaction state==doing)
+# cannot be reproduced deterministically here: /test/compact submits a
background job and returns
+# immediately, so the race window is too narrow to hit reliably from a shell
script.
+# Save-side coverage lives in the unit test: HgSnapshotHandlerTest.
+#
+# Requires: Docker Desktop >= 20.10, >= 12 GB allocated to Docker, Docker
Compose v2
+# Run from the repo root:
+# bash docker/hbase/test/test-snapshot-corruption.sh # confirm
load-path bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm
load-path fix is active (fixed image)
+
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)"
+COMPOSE_FILE="$SCRIPT_DIR/../docker-compose-3pd-3store-3server.yml"
+HUGEGRAPH_VERSION="${HUGEGRAPH_VERSION:-1.7.0}"
+VOLUME_PREFIX="hugegraph-3x3"
+STORE_LOG="hugegraph-store.log"
+FIXED_MODE=false
+[[ "${1:-}" == "--fixed" ]] && FIXED_MODE=true
+
+RED='\033[0;31m'; GREEN='\033[0;32m'; YELLOW='\033[1;33m'; NC='\033[0m'
+log() { echo -e "${GREEN}[repro]${NC} $*"; }
+warn() { echo -e "${YELLOW}[repro]${NC} $*"; }
+fail() { echo -e "${RED}[repro] FAIL${NC} $*" >&2; exit 1; }
+
+JAR_SOURCE="$REPO_ROOT/hugegraph-store/hg-store-node/target/hg-store-node-${HUGEGRAPH_VERSION}.jar"
+
+if $FIXED_MODE; then
+ STORE_IMAGE="${STORE_IMAGE:-hugegraph/store:patched}"
+ if [[ "$STORE_IMAGE" == "hugegraph/store:patched" ]] && \
+ ! docker image inspect "hugegraph/store:patched" >/dev/null 2>&1; then
+ log "Patched image not found — building from source..."
+ if [[ ! -f "$JAR_SOURCE" ]]; then
+ log " Compiling hugegraph-store (this takes a minute)..."
+ mvn package -pl hugegraph-store/hg-store-node -am -DskipTests -q \
+ -f "$REPO_ROOT/pom.xml"
+ fi
+ BUILD_CTX="$(mktemp -d)"
+ cp "$JAR_SOURCE" "$BUILD_CTX/hg-store-node-${HUGEGRAPH_VERSION}.jar"
+ cat > "$BUILD_CTX/Dockerfile" <<EOF
+FROM hugegraph/store:${HUGEGRAPH_VERSION}
+COPY hg-store-node-${HUGEGRAPH_VERSION}.jar
/hugegraph-store/lib/hg-store-node-${HUGEGRAPH_VERSION}.jar
+EOF
+ docker build -t "hugegraph/store:patched" "$BUILD_CTX" >/dev/null
+ log " Built hugegraph/store:patched."
+ fi
+else
+ STORE_IMAGE="${STORE_IMAGE:-hugegraph/store:${HUGEGRAPH_VERSION}}"
+fi
+
+log "Store image: $STORE_IMAGE (fixed-mode: $FIXED_MODE)"
+log "Compose File: $COMPOSE_FILE"
+
+# If a non-default store image is requested, write a temporary compose
override that
+# replaces the store image — without modifying the committed compose file.
+OVERRIDE_FILE=""
+if [ "$STORE_IMAGE" != "hugegraph/store:${HUGEGRAPH_VERSION}" ]; then
+ OVERRIDE_FILE="$(mktemp /tmp/snapshot-test-override-XXXXXX.yml)"
+ cat > "$OVERRIDE_FILE" <<EOF
+services:
+ store0:
+ image: ${STORE_IMAGE}
+ store1:
+ image: ${STORE_IMAGE}
+ store2:
+ image: ${STORE_IMAGE}
+EOF
+ log " Using override file $OVERRIDE_FILE for image $STORE_IMAGE"
+fi
+# Build the compose -f arguments (base file always first; override appended
when set)
+COMPOSE_ARGS="-f $COMPOSE_FILE"
+[ -n "$OVERRIDE_FILE" ] && COMPOSE_ARGS="$COMPOSE_ARGS -f $OVERRIDE_FILE"
+# Clean up temp files on exit (BUILD_CTX trap already set in --fixed mode
above)
+trap '[ -n "$OVERRIDE_FILE" ] && rm -f "$OVERRIDE_FILE"; [ -n "${BUILD_CTX:-}"
] && rm -rf "$BUILD_CTX"' EXIT
+
+wait_http() {
+ local url=$1 label=$2 tries=${3:-60}
+ log "Waiting for $label..."
+ for i in $(seq 1 "$tries"); do
+ if curl -fsS "$url" >/dev/null 2>&1; then log "$label up."; return 0;
fi
+ sleep 3
+ done
+ fail "$label not healthy after $((tries * 3))s"
+}
+
+# ── Step 1: Start cluster
─────────────────────────────────────────────────────
+log "Step 1: Tearing down any previous run and starting a clean cluster..."
+HUGEGRAPH_VERSION="$HUGEGRAPH_VERSION" \
+ docker compose $COMPOSE_ARGS down -v --remove-orphans 2>&1 | tail -3 ||
true
+HUGEGRAPH_VERSION="$HUGEGRAPH_VERSION" \
+ docker compose $COMPOSE_ARGS up -d \
+ --scale server0=0 --scale server1=0 --scale server2=0 \
+ 2>&1 | grep -E "Started|Healthy|healthy" | tail -5 || true
+
+wait_http "http://localhost:8620/v1/health"; "pd0" 60
+wait_http "http://localhost:8520/v1/health"; "store0" 60
+wait_http "http://localhost:8521/v1/health"; "store1" 60
+wait_http "http://localhost:8522/v1/health"; "store2" 60
+
+# Raft partition dirs are created lazily when the server first registers a
graph.
+# Start server0 just long enough for init-store to run, then stop it.
+# We only need the init_complete flag to be written — we do NOT wait for
/versions
+# because start-hugegraph.sh has a 120s JVM-ready timeout that can expire on a
cold
+# distributed cluster, causing the entrypoint to exit and Docker to restart the
+# container, resetting the timer indefinitely.
+if ! docker exec hg-store0 sh -c 'ls /hugegraph-store/storage/raft/
2>/dev/null | grep -qE "^[0-9]{5}$"' 2>/dev/null; then
+ log " Fresh cluster: starting server0 briefly to initialise partitions..."
+ HUGEGRAPH_VERSION="$HUGEGRAPH_VERSION" \
+ HUGEGRAPH_STORE_IMAGE="$STORE_IMAGE" \
+ docker compose $COMPOSE_ARGS up -d server0 2>&1 | tail -2 || true
+
+ log " Waiting for init-store to complete (up to 120s)..."
+ for i in $(seq 1 40); do
+ if docker exec hg-server0 test -f
/hugegraph-server/docker/init_complete 2>/dev/null; then
+ log " init_complete flag found after ~$((i * 3))s."
+ break
+ fi
+ sleep 3
+ done
+ docker exec hg-server0 test -f /hugegraph-server/docker/init_complete
2>/dev/null \
+ || fail "init-store did not complete within 120s"
+
+ # Give Raft groups a moment to create their partition dirs
+ sleep 5
+ docker compose $COMPOSE_ARGS stop server0 2>/dev/null || true
+ log " server0 stopped — partitions initialised."
+fi
+
+# ── Step 2: Ensure committed snapshots exist on store0 ───────────────────────
+log "Step 2: Flushing + snapshotting all store nodes..."
+for port in 8520 8521 8522; do
+ curl -fsS "http://localhost:${port}/test/flush"; >/dev/null && log "
:${port} flush OK"
+ curl -fsS "http://localhost:${port}/test/snapshot"; >/dev/null && log "
:${port} snapshot triggered"
+done
+log "Waiting 20s for Raft snapshot commits..."
+sleep 20
+
+SNAP_COUNT=$(docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c 'find /hugegraph-store/storage/raft -name "data" -type d |
wc -l')
+log "store0 has $SNAP_COUNT committed snapshot data/ directories."
+[ "$SNAP_COUNT" -ge 1 ] || fail "No committed snapshots on store0. Retry."
+
+# ── Step 3: Stop all stores
───────────────────────────────────────────────────
+log "Step 3: Stopping all store nodes..."
+docker stop hg-store0 hg-store1 hg-store2 >/dev/null
+log "All stores stopped."
+
+# ── Step 4: Corrupt one partition snapshot on store0 ─────────────────────────
+# Two sub-cases of the bug:
+#
+# Sub-case A (race: state==doing at snapshot-save time) — load-path simulated
in default mode:
+# The actual race cannot be triggered deterministically from a shell script
(see header).
+# We instead simulate the outcome: manually remove data/ and
should_not_load, leaving only
+# __raft_snapshot_meta, which is what the race would produce.
+# On load: goes straight to loadSnapshot(missingDir) → "not exists" → stuck.
+# Fix 1 (throw instead of return in onSnapshotSave) prevents this snapshot
from ever being
+# committed; this script validates only the resulting load-path error, not
the throw itself.
+#
+# Sub-case B (JVM killed after should_not_load but before data/ completes) —
tested in --fixed mode:
+# Snapshot dir has __raft_snapshot_meta + should_not_load, but no data/.
+# On load (buggy): shouldNotLoad() == true → silent return → partition
silently has no data.
+# On load (fixed): Fix 2 detects data/ is missing → logs warning → falls
through to
+# loadSnapshot → throws "not exists" → JRaft signals error
→ leader rescues.
+#
+log "Step 4: Corrupting one snapshot on store0 (sub-case $( $FIXED_MODE &&
echo B || echo A ))..."
+TARGET=$(docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c '
+ for meta in $(find /hugegraph-store/storage/raft -name
"__raft_snapshot_meta" | sort); do
+ snap=$(dirname "$meta")
+ if [ -d "$snap/data" ] && [ -f "$snap/should_not_load" ]; then
+ echo "$snap"; break
+ fi
+ done
+ ')
+
+[ -n "$TARGET" ] || fail "No suitable snapshot found (need data/ +
should_not_load + __raft_snapshot_meta)"
+
+PARTITION_ID=$(echo "$TARGET" | grep -oE '/[0-9]{5}/' | head -1 | tr -d '/')
Review Comment:
🧹 This line can abort the whole run with no diagnostic.
The script sets `set -euo pipefail` (line 35). If `$TARGET` has no
five-digit path segment, `grep -oE '/[0-9]{5}/'` exits 1, `pipefail` surfaces
that as the pipeline status even though `head` and `tr` succeed, and the
failing command substitution in a simple assignment trips `set -e` — the script
exits silently right after reporting `Target: partition ... `, with the cluster
left running and one snapshot already about to be corrupted.
Requested change: tolerate the miss and fail loudly instead, e.g.
```bash
PARTITION_ID=$(echo "$TARGET" | grep -oE '/[0-9]{5}/' | head -1 | tr -d '/'
|| true)
[ -n "$PARTITION_ID" ] || fail "Could not derive partition id from snapshot
path: $TARGET"
```
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/core/snapshot/HgSnapshotHandlerTest.java:
##########
@@ -42,13 +57,162 @@ public class HgSnapshotHandlerTest extends
StoreEngineTestBase {
private static HgSnapshotHandler hgSnapshotHandlerUnderTest;
+ @Rule
+ public TemporaryFolder tmpDir = new TemporaryFolder();
+
@Before
public void setUp() throws IOException {
hgSnapshotHandlerUnderTest = new
HgSnapshotHandler(createPartitionEngine(0));
FileUtils.forceMkdir(new File("/tmp/snapshot"));
FileUtils.forceMkdir(new File("/tmp/snapshot/data"));
}
+ // ── Fix 1: onSnapshotSave must throw when compaction is in progress
────────
+
+ /**
+ * Before the fix, onSnapshotSave silently returned when state == doing,
+ * causing JRaft to commit an empty snapshot dir with no data/.
+ * After the fix it must throw HgStoreException so JRaft retries instead.
+ */
+ @Test
+ public void testOnSnapshotSaveThrowsWhenCompactionInProgress() {
+ // Build a SnapshotHandler wired to a mock PartitionEngine whose
BusinessHandler
+ // reports state == doing (compaction active) for partition 0.
+ PartitionEngine mockEngine = mock(PartitionEngine.class);
+ HgStoreEngine mockStoreEngine = mock(HgStoreEngine.class);
+ BusinessHandler mockBusinessHandler = mock(BusinessHandler.class);
+
+ AtomicInteger doingState = new AtomicInteger(BusinessHandler.doing);
+
+ when(mockEngine.getGroupId()).thenReturn(0);
+ when(mockEngine.getStoreEngine()).thenReturn(mockStoreEngine);
+
when(mockStoreEngine.getBusinessHandler()).thenReturn(mockBusinessHandler);
+ when(mockBusinessHandler.getState(0)).thenReturn(doingState);
+
+ SnapshotHandler handler = new SnapshotHandler(mockEngine);
+
+ SnapshotWriter stubWriter = stubWriter("/tmp/snapshot");
+
+ HgStoreException ex = assertThrows(
+ "onSnapshotSave must throw when state == doing",
+ HgStoreException.class,
+ () -> handler.onSnapshotSave(stubWriter));
+
+ assertTrue("Exception message must mention the partition",
+ ex.getMessage().contains("0"));
+ assertTrue("Exception message must describe the cause",
+ ex.getMessage().contains("compaction in progress"));
+ }
+
+ /**
+ * When state is NOT doing (e.g. compactionDone), onSnapshotSave must not
throw.
+ */
+ @Test
+ public void testOnSnapshotSaveDoesNotThrowWhenNotBusy() throws Exception {
+ PartitionEngine mockEngine = mock(PartitionEngine.class);
+ HgStoreEngine mockStoreEngine = mock(HgStoreEngine.class);
+ BusinessHandler mockBusinessHandler = mock(BusinessHandler.class);
+
+ // state == compactionDone (not doing) — save should proceed normally
+ AtomicInteger doneState = new
AtomicInteger(BusinessHandler.compactionDone);
+
+ when(mockEngine.getGroupId()).thenReturn(0);
+ when(mockEngine.getStoreEngine()).thenReturn(mockStoreEngine);
+
when(mockStoreEngine.getBusinessHandler()).thenReturn(mockBusinessHandler);
+ when(mockBusinessHandler.getState(0)).thenReturn(doneState);
+
+ // saveSnapshot is a no-op via the mock, so we just need it not to
throw at the guard
+ SnapshotHandler handler = new SnapshotHandler(mockEngine);
+ SnapshotWriter stubWriter =
stubWriter(tmpDir.newFolder("snap-not-busy").getAbsolutePath());
+
+ // No exception should propagate from the state guard.
+ // (saveSnapshot will throw because the mock returns null for it —
that's fine,
+ // we only care the doing-check is not hit.)
+ try {
Review Comment:
🧹 This test asserts nothing on its success path, and the new tests sit in
the wrong test class.
`handler.onSnapshotSave(stubWriter)` is wrapped in `try { ... } catch
(HgStoreException e)` with the only assertion inside the catch, so if no
exception is thrown the test passes without checking anything — and it would
still pass if the compaction guard at `SnapshotHandler.java:98-102` were
deleted outright, since the guard is not the thing being exercised on this path.
Separately, all four new tests construct `SnapshotHandler`, but the class is
`HgSnapshotHandlerTest` and its `setUp` builds `new
HgSnapshotHandler(createPartitionEngine(0))` (line 65) for the pre-existing
tests. With two near-identical handler classes in the same package — only
`SnapshotHandler` is wired into production, at `PartitionEngine.java:176` —
mixing them in one file makes it hard to tell which one any given test covers.
Requested change: assert positively that the non-busy path proceeds (for
example `verify(mockBusinessHandler).saveSnapshot(anyString(), eq(""), eq(0))`)
rather than swallowing the exception, and move the four `SnapshotHandler` tests
into their own `SnapshotHandlerTest`.
--
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]
