You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "vaijosh (via GitHub)" <gi...@apache.org> on 2026/08/18 10:37:11 UTC
[PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
vaijosh opened a new pull request, #3164:
URL: https://github.com/apache/hugegraph/pull/3164
## Purpose of the PR
- close #3162
This PR addresses a race condition that occurs during snapshot saves when compaction is busy, which could previously lead to corrupted snapshots or stuck partitions.
## Main Changes
- **Throw on compaction-busy:** Modified the snapshot save behavior to throw an exception rather than returning early/silently failing when compaction is busy.
- **Validate `data/` on load:** Added validation during the snapshot load process to verify the presence of the `data/` directory, preventing the system from loading incomplete snapshots.
- **Reproduction Script:** Added `test-snapshot-corruption.sh` to deterministically reproduce the bug and validate the fix across different storage states.
- **Unit Tests:** Added UTs to cover the new validation logic and race condition handling.
## Verifying these changes
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests here)*.
- [x] Need tests and can be verified as follows:
- Execute the newly added unit tests.
- Run the `test-snapshot-corruption.sh` script to verify the corrupted snapshot detection and prevention.
## Does this PR potentially affect the following parts?
- [ ] Dependencies
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
- [x] Nope
## Documentation Status
- [ ] `Doc - TODO`
- [ ] `Doc - Done`
- [x] `Doc - No Need`
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3816134782
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,311 @@
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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) — tested in default mode:
+# onSnapshotSave returns early → neither data/ nor should_not_load written.
+# Snapshot dir has only __raft_snapshot_meta.
+# On load: goes straight to loadSnapshot(missingDir) → "not exists" → stuck.
+# Fix 1 (throw instead of return) prevents this snapshot from ever being committed.
+#
+# 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 '/')
+SNAP_NAME=$(basename "$TARGET")
+
+if $FIXED_MODE; then
+ # Sub-case B: remove only data/, keep should_not_load.
+ # Buggy image: shouldNotLoad() fires, silently returns — no error logged.
+ # Fixed image (Fix 2): detects data/ missing, logs warning, falls through.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ only — keeping should_not_load (sub-case B)"
+ docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c "rm -rf '${TARGET}/data'
+ echo 'Contents after corruption:'; ls '${TARGET}'"
+else
+ # Sub-case A: remove both data/ and should_not_load — exactly what the race produces.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ and should_not_load — leaving only __raft_snapshot_meta (sub-case A)"
+ docker run --rm \
Review Comment:
⚠️ Blocking: no. The default mode does not exercise the compaction-busy save path: it first creates a completed snapshot, then deletes `data/` and `should_not_load` at lines 211-214 and labels the resulting load error as `BUG REPRODUCED`. The fixed implementation would hit the same injected corruption, while `--fixed` only checks the load-side warning. Please add a run that triggers snapshot while compaction is actually active and asserts the save failure/retry, or narrow the script's claim. Evidence: lines 168-178 and 211-214; `/test/snapshot` only invokes `PartitionEngine.snapshot()` and the script never invokes `/test/compact`.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3940032346
##########
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:
Addressed. Thanks @bitflicker64
##########
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:
Addressed. Thanks @imbajin
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5647345766
> ### The race reproduces deterministically
> On a 3-VM lab (1 PD + 3 Store + 1 Server, replication 1, 25 M edges, partitions of 60–275 MB) the window is not narrow: a full compaction of a partition holding ~250 MB of unflushed data takes 7 s, and a snapshot requested 300 ms after it lands in `state == doing` every time. The compaction is started blocking through `POST :8520/v1/compat?id=`, the snapshot through `GET :8520/test/snapshot`. Script: [`cluster/race_snapshot_compaction.sh`](https://github.com/SebastianGruza/hugegraph-validation/blob/master/cluster/race_snapshot_compaction.sh), store and jraft logs in [`results/issue-3162/`](https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3162).
>
> The only case where the window really disappears is a freshly compacted partition: `compactRange` on 62 MB with nothing to do takes 0.1 s.
>
> ### What master does, step by step
> step master `60c8803` PR head `1c6bc23`
> snapshot during the compaction `onSnapshotSave success` in the log, directory `snapshot_50210` with only `__raft_snapshot_meta`, no `data/`, no `should_not_load`; jraft immediately deletes the previous good snapshot and truncates the log prefix to 50209 `snapshot save failed: compaction in progress`, status EBUSY, no directory at all; the compaction finishes normally and the snapshot after it is valid
> `SIGKILL` the store 1.3 s after the snapshot, restart without wiping `onSnapshotLoad failed` → `initSnapshotStorage failed` → `Raft 15 is restarting !!!` once, then silence; the partition stays dead same, only the exception text changes (`snapshot is corrupt, data dir …`)
> what clients see meanwhile `g.E().count()` = 24 045 646 of 25 000 000, `g.V().count()` = 97 185 of 100 000, HTTP 200, no error on the server side; store REST `/v1/partition/15` returns 500 same
> `rm -rf snapshot_50210`, restart `Missing logs in (0, 50209)`: the log prefix is gone, the partition stays dead same
> `rm -rf raft/00015` (log, meta, snapshot), keep `db/00015`, restart the partition comes back (100 000 vertices) but **257 001 edges are missing**: HStore runs RocksDB with `setDisableWAL(true)`, the raft log is the WAL, so the unflushed memtable of the killed process is lost together with the log
> ### Two things this says about the PR
> * **The save side does exactly what is needed.** With the fix no directory without `data/` is created, and more importantly the previous good snapshot is not deleted and the log is not truncated. That is the fix that matters, because once an empty snapshot is committed there is no way back: jraft has removed the previous snapshot and the log prefix, and the local RocksDB has no WAL.
> * **The load side detects but does not rescue.** The `data/` validation only changes the message; the partition still does not come up and the server keeps answering 200 with partial results. That is not a complaint about the PR, rather a hint that "what to do with a snapshot written before the fix" deserves its own issue: for replication 1 I know no procedure that does not lose data; for replication ≥ 2 a re-install from the leader is probably the natural one.
>
> ### Numbers for the 10 s discussion
> `onSnapshotSave` on a 274 MB partition completes within the same log second (the checkpoint is hard links, CRC64 over the first and last 4 KB of each file), so the "well under a second" assumption holds. The compaction with 250 MB of unflushed data took 7 s, so with `snapshotInterval 1800` a collision is rare, but with a compaction requested after a bulk load it will certainly happen; EBUSY instead of EIO matters there.
>
> If it helps, I can repeat the same runs on the next PR head or at replication 3 (where the interesting part is what a follower installing a snapshot in that window receives).
Thanks, @SebastianGruza. Yes, this will really help!
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3818659311
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,311 @@
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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) — tested in default mode:
+# onSnapshotSave returns early → neither data/ nor should_not_load written.
+# Snapshot dir has only __raft_snapshot_meta.
+# On load: goes straight to loadSnapshot(missingDir) → "not exists" → stuck.
+# Fix 1 (throw instead of return) prevents this snapshot from ever being committed.
+#
+# 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 '/')
+SNAP_NAME=$(basename "$TARGET")
+
+if $FIXED_MODE; then
+ # Sub-case B: remove only data/, keep should_not_load.
+ # Buggy image: shouldNotLoad() fires, silently returns — no error logged.
+ # Fixed image (Fix 2): detects data/ missing, logs warning, falls through.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ only — keeping should_not_load (sub-case B)"
+ docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c "rm -rf '${TARGET}/data'
+ echo 'Contents after corruption:'; ls '${TARGET}'"
+else
+ # Sub-case A: remove both data/ and should_not_load — exactly what the race produces.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ and should_not_load — leaving only __raft_snapshot_meta (sub-case A)"
+ docker run --rm \
Review Comment:
@imbajin
You're correct. The save-side throw (SnapshotHandler.java:98–101) only fires if compaction is actually in flight when snapshot() is called, and we have no reliable way to hold that state long enough. Because /test/compact submits a background task and returns immediately, the race window is too narrow to hit deterministically. This would make the test flaky.
Currently, the script simulates the outcome of that race (a missing data/ dir) by removing it manually, which lets us at least verify the load-side behavior. I have added a comment in the script to make this clear.
If there is a way to deterministically reproduce the save-side throw, I would be happy to incorporate it into the script.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5651805274
> Blocking: no. Summary: The previous rounds' findings are resolved at this head. Snapshot save and `compactRange()` now share a per-partition `ReentrantLock`, a busy save reports `RaftError.EBUSY` instead of restarting the raft node, a compaction that meets a running save waits up to 10s before skipping with a WARN, and the `data/` check sits after the `should_not_load` return. Evidence: exact-head diff and the incremental diff `468e2343..1c6bc23a` (`BusinessHandlerImpl.java:1440-1463`, `SnapshotHandler.java`, `PartitionStateMachine.java`); all 24 checks on `1c6bc23a` pass, and the `store` job log shows `CoreSuiteTest` (8 tests, including the skip path: `Partition 4 skip dbCompaction ... after 200ms wait`), `BatchGraphIsolationTest` (6) and `RaftSuiteTest` (6) passing. Optional nit: the comment at `BusinessHandlerImpl.java:1446-1449` still says the next compaction pass will succeed, but the event-driven triggers are one-shot.
Thanks @bitflicker64. I have fixed the comment at `BusinessHandlerImpl.java:1446-1449`.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3896231298
##########
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:
I have removed `test-snapshot-corruption.sh` because I was unable to reproduce the original issue (the `saveSnapshot` race condition) using it. Instead, I have covered those scenarios with unit tests in a more deterministic way.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3813404467
##########
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 String dataDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;
+ if (new File(dataDir).exists()) {
Review Comment:
Thanks @imbajin
I have fixed it.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3805553518
##########
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 String dataDir = snapshotDir + File.separator + SNAPSHOT_DATA_PATH;
+ if (new File(dataDir).exists()) {
Review Comment:
⚠️ Blocking: yes. `File.exists()` is also true when `data` is a regular file. A snapshot containing `should_not_load` plus a file named `data` still takes this early return and hides the corruption instead of reaching `loadSnapshot`; use `isDirectory()` (and validate the expected contents) before skipping. Evidence: `SnapshotHandler.java:176-185` at head `7ee5d420ca7301d8038f1fe981dd82623c8961c9`.
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/usr/bin/env bash
Review Comment:
🧹 Blocking: no. The file starts with a blank byte before `#!/usr/bin/env bash`, so direct execution of this 100755 script can fail with `Exec format error`; move the shebang to byte 0. Evidence: the exact-head first bytes are `0a 23 21 2f`.
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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"
Review Comment:
⚠️ Blocking: no. With this script under `docker/test`, `$SCRIPT_DIR/../../docker-compose-3pd-3store-3server.yml` resolves to `<repo>/docker-compose-3pd-3store-3server.yml`, but the exact-head tree contains the compose file at `docker/docker-compose-3pd-3store-3server.yml`; the first `docker compose` command therefore fails in both modes. The same path block sets `REPO_ROOT` with `../../../..` above the repository, so `--fixed` also cannot find `pom.xml`. Please correct the relative paths and the stale `docker/hbase/test` examples. Evidence: exact-head tree listing and path resolution.
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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; }
+
+# In --fixed mode use the locally-built patched image.
+# Build it from source if it doesn't exist yet so the caller only needs --fixed.
+PATCHED_IMAGE="hugegraph/store:patched"
+DOCKERFILE="$SCRIPT_DIR/../../Dockerfile.store-patched"
Review Comment:
‼️ Blocking: no. A clean `--fixed` run defaults to `hugegraph/store:patched`, then unconditionally builds with `.../Dockerfile.store-patched` when that image is absent, but the exact-head tree contains no `Dockerfile.store-patched` under `docker/` or the repository root. This makes the documented fixed mode fail before the reproducer starts; add the Dockerfile or point to an existing build path. Evidence: `gh api repos/apache/hugegraph/contents/docker?ref=7ee5d420ca7301d8038f1fe981dd82623c8961c9` lists no such file.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4012562834
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1438,28 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ if (!rangeLock.tryLock(compactionRangeLockWaitMillis,
Review Comment:
Thanks @imbajin . I have fixed this in recent commit.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4035327842
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandler.java:
##########
@@ -230,6 +230,14 @@ void lock(String path) throws InterruptedException,
void unlock(String path);
+ /**
+ * Non-blocking attempt to reserve the compactRange() window for partition {@code id}.
+ * Returns false if a compaction is actively running for that partition right now.
+ */
+ boolean tryLockCompactionRange(int id);
Review Comment:
Addressed.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1413,30 +1456,58 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
pathLock.putIfAbsent(path, new AtomicInteger(compactionCanStart));
compactionState.putIfAbsent(id, new AtomicInteger(0));
log.info("Partition {} dbCompaction started", id);
- if (tableName.isEmpty()) {
- lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
- log.info("Partition {} dbCompaction end and start to do snapshot", id);
- PartitionEngine pe = HgStoreEngine.getInstance().getPartitionEngine(id);
- // find leader and send blankTask, after execution
- if (pe.isLeader()) {
- RaftClosure bc = (closure) -> {
- };
- pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
- bc);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked = rangeLock.tryLock(compactionRangeLockWaitMillis,
+ TimeUnit.MILLISECONDS);
+ if (rangeLocked) {
+ if (tableName.isEmpty()) {
+ lock(path);
+ setState(id, doing);
+ log.info("Partition {}-{} got lock, dbCompaction start", id, path);
+ op.compactRange();
+ setState(id, compactionDone);
+ log.info("Partition {} dbCompaction end and start to do snapshot", id);
+ PartitionEngine pe = HgStoreEngine.getInstance().getPartitionEngine(id);
+ // find leader and send blankTask, after execution
+ if (pe.isLeader()) {
+ RaftClosure bc = (closure) -> {
+ };
+ pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
+ bc);
+ } else {
+ HgCmdClient client = HgStoreEngine.getInstance().getHgCmdClient();
+ BlankTaskRequest request = new BlankTaskRequest();
+ request.setGraphName("");
+ request.setPartitionId(id);
+ client.tryInternalCallSyncWithRpc(request);
+ }
+ setAndNotifyState(id, compactionDone);
+ } else {
+ op.compactRange(tableName);
+ }
} else {
- HgCmdClient client = HgStoreEngine.getInstance().getHgCmdClient();
- BlankTaskRequest request = new BlankTaskRequest();
- request.setGraphName("");
- request.setPartitionId(id);
- client.tryInternalCallSyncWithRpc(request);
+ log.warn("Partition {} skip dbCompaction, snapshot save " +
+ "still in progress after {}ms wait", id,
+ compactionRangeLockWaitMillis);
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ log.warn("Partition {} dbCompaction interrupted while waiting " +
+ "for snapshot range lock", id);
+ // Interrupted while waiting for the snapshot save to release
+ // the range lock. The path lock was already acquired and
+ // must be released here, otherwise later compactions for this
+ // partition would block until the path lock timeout.
+ unlock(path);
Review Comment:
Addressed
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3988516984
##########
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)
+@Suite.SuiteClasses({
+ HgSnapshotHandlerTest.class
Review Comment:
Addressed Review comments.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4035330032
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1190,15 +1212,36 @@ public boolean cleanPartition(String graph, int partId, long startKey, long endK
taskManager.putAsyncTask(cleanTask);
Utils.runInThread(() -> {
- cleanPartition(partition, code -> {
- // in range
- boolean flag = code >= startKey && code < endKey;
- return (cleanType == CleanType.CLEAN_TYPE_KEEP_RANGE) == flag;
- });
- // May have been destroyed.
- if (HgStoreEngine.getInstance().getPartitionEngine(partId) != null) {
- taskManager.updateAsyncTaskState(partId, graph, cleanTask.getId(),
- AsyncTaskState.SUCCESS);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(partId, k -> new ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked = rangeLock.tryLock(compactionRangeLockWaitMillis,
Review Comment:
Addressed
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/raft/PartitionStateMachine.java:
##########
@@ -198,7 +198,13 @@ public void onSnapshotSave(final SnapshotWriter writer, final Closure done) {
done.run(Status.OK());
} catch (HgStoreException e) {
log.error(String.format("Raft %s onSnapshotSave failed. {}", groupId), e);
- done.run(new Status(RaftError.EIO, e.toString()));
+ // A busy compaction-range lock is transient: jRaft's snapshot scheduler
+ // retries independently, so report EBUSY rather than EIO to avoid
+ // escalating to reportError()/restartRaftNode() (see SnapshotExecutorImpl
+ // #onSnapshotSaveDone, which only escalates on EIO).
+ RaftError raftError = e.getCode() == HgStoreException.EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL ?
Review Comment:
Addressed
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3999405850
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -154,6 +165,18 @@ public class BusinessHandlerImpl implements BusinessHandler {
private final InnerKeyCreator keyCreator;
private final Semaphore semaphore = new Semaphore(1);
+ /* Bounds how long dbCompaction() waits to acquire compactionRangeLock when a snapshot
+ save is holding it. saveSnapshot() is a RocksDB Checkpoint (hard-links existing SST
+ files, no data copy) plus a partial checksum read, so the lock is normally held for
+ well under a second, at most a few seconds under a slow/busy disk. 10s gives generous
+ margin over that expected hold time while keeping a stuck snapshot save from blocking
+ compaction for long: if the wait is exceeded, dbCompaction just skips this pass and
+ relies on the next trigger (PD instruction, REST call, etc.) to retry - see the
Review Comment:
🧹 This still says a skipped pass "relies on the next trigger (PD instruction, REST call, etc.) to retry", but those triggers are one-shot (`PartitionEngine.java:1017,1263`, `HgStoreEngine.java:505`, `PartitionAPI.java:207`), so a skipped compaction is dropped. The last commit removed the same claim from the inline comment at 1446-1448, but that comment now ends mid-sentence ("rather than block - callers of dbCompaction().").
Could you say here that the request is dropped with a WARN and not retried, and finish the inline sentence (e.g. "rather than block the compaction pool thread")?
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "SebastianGruza (via GitHub)" <gi...@apache.org>.
SebastianGruza commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5647150080
### The race reproduces deterministically
On a 3-VM lab (1 PD + 3 Store + 1 Server, replication 1, 25 M edges, partitions of 60–275 MB) the window is not narrow: a full compaction of a partition holding ~250 MB of unflushed data takes 7 s, and a snapshot requested 300 ms after it lands in `state == doing` every time. The compaction is started blocking through `POST :8520/v1/compat?id=`, the snapshot through `GET :8520/test/snapshot`. Script: [`cluster/race_snapshot_compaction.sh`](https://github.com/SebastianGruza/hugegraph-validation/blob/master/cluster/race_snapshot_compaction.sh), store and jraft logs in [`results/issue-3162/`](https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3162).
The only case where the window really disappears is a freshly compacted partition: `compactRange` on 62 MB with nothing to do takes 0.1 s.
### What master does, step by step
| step | master `60c8803` | PR head `1c6bc23` |
|---|---|---|
| snapshot during the compaction | `onSnapshotSave success` in the log, directory `snapshot_50210` with only `__raft_snapshot_meta`, no `data/`, no `should_not_load`; jraft immediately deletes the previous good snapshot and truncates the log prefix to 50209 | `snapshot save failed: compaction in progress`, status EBUSY, no directory at all; the compaction finishes normally and the snapshot after it is valid |
| `SIGKILL` the store 1.3 s after the snapshot, restart without wiping | `onSnapshotLoad failed` → `initSnapshotStorage failed` → `Raft 15 is restarting !!!` once, then silence; the partition stays dead | same, only the exception text changes (`snapshot is corrupt, data dir …`) |
| what clients see meanwhile | `g.E().count()` = 24 045 646 of 25 000 000, `g.V().count()` = 97 185 of 100 000, HTTP 200, no error on the server side; store REST `/v1/partition/15` returns 500 | same |
| `rm -rf snapshot_50210`, restart | `Missing logs in (0, 50209)`: the log prefix is gone, the partition stays dead | same |
| `rm -rf raft/00015` (log, meta, snapshot), keep `db/00015`, restart | the partition comes back (100 000 vertices) but **257 001 edges are missing**: HStore runs RocksDB with `setDisableWAL(true)`, the raft log is the WAL, so the unflushed memtable of the killed process is lost together with the log | |
### Two things this says about the PR
- **The save side does exactly what is needed.** With the fix no directory without `data/` is created, and more importantly the previous good snapshot is not deleted and the log is not truncated. That is the fix that matters, because once an empty snapshot is committed there is no way back: jraft has removed the previous snapshot and the log prefix, and the local RocksDB has no WAL.
- **The load side detects but does not rescue.** The `data/` validation only changes the message; the partition still does not come up and the server keeps answering 200 with partial results. That is not a complaint about the PR, rather a hint that "what to do with a snapshot written before the fix" deserves its own issue: for replication 1 I know no procedure that does not lose data; for replication ≥ 2 a re-install from the leader is probably the natural one.
### Numbers for the 10 s discussion
`onSnapshotSave` on a 274 MB partition completes within the same log second (the checkpoint is hard links, CRC64 over the first and last 4 KB of each file), so the "well under a second" assumption holds. The compaction with 250 MB of unflushed data took 7 s, so with `snapshotInterval 1800` a collision is rare, but with a compaction requested after a bulk load it will certainly happen; EBUSY instead of EIO matters there.
If it helps, I can repeat the same runs on the next PR head or at replication 3 (where the interesting part is what a follower installing a snapshot in that window receives).
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4002175679
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1438,28 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ if (!rangeLock.tryLock(compactionRangeLockWaitMillis,
Review Comment:
⚠️ Blocking: no. `tryLock(long, TimeUnit)` is interruptible, but this task has already changed `pathLock` to `doing` in `lock(path)`. An interrupt therefore jumps to the outer catch at lines 1484-1485 without calling `unlock(path)`; later compactions for this partition wait until the six-hour `timeoutMillis` expires. Please handle `InterruptedException` with an abort/finally cleanup that releases `path`, while keeping the path lock held for the successful compaction-to-blank-task handoff.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4024558796
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -138,6 +143,12 @@ public class BusinessHandlerImpl implements BusinessHandler {
private static final ConcurrentMap<String, AtomicInteger> pathLock = new ConcurrentHashMap<>();
private static final ConcurrentMap<Integer, AtomicInteger> compactionState =
new ConcurrentHashMap<>();
+ // Guards the compactRange() window specifically, so a snapshot save can atomically
+ // check-and-reserve against a compaction that is actually running right now. This is
+ // narrower than pathLock, which stays held through the post-compaction blank-task
+ // snapshot and must not be reused here to avoid deadlocking that flow.
+ private static final ConcurrentMap<Integer, ReentrantLock> compactionRangeLock =
Review Comment:
Thanks @bitflicker64. I have fixed it.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4000153725
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -154,6 +165,18 @@ public class BusinessHandlerImpl implements BusinessHandler {
private final InnerKeyCreator keyCreator;
private final Semaphore semaphore = new Semaphore(1);
+ /* Bounds how long dbCompaction() waits to acquire compactionRangeLock when a snapshot
+ save is holding it. saveSnapshot() is a RocksDB Checkpoint (hard-links existing SST
+ files, no data copy) plus a partial checksum read, so the lock is normally held for
+ well under a second, at most a few seconds under a slow/busy disk. 10s gives generous
+ margin over that expected hold time while keeping a stuck snapshot save from blocking
+ compaction for long: if the wait is exceeded, dbCompaction just skips this pass and
+ relies on the next trigger (PD instruction, REST call, etc.) to retry - see the
Review Comment:
@bitflicker64
I think the wording in the comment is correct. It says:
dbCompaction just skips this pass -- Means “If the lock wait is exceeded, dbCompaction() gives up on that attempt and skips the current pass.”
“It relies on the next trigger (PD instruction, REST call, etc.).” -- Means that compaction does not spin or schedule an immediate internal retry in that code path. If tryLock() times out, the current attempt ends, and a later external trigger—such as a PD instruction, REST call, or another compaction request—can start a new attempt.
Nowhere do we mention an “internal retry in that code path.”
Earlier I had removed the line because it looked redundant at that place.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "SebastianGruza (via GitHub)" <gi...@apache.org>.
SebastianGruza commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5677119046
Follow-up to the measurement of Sep 12, same lab, now with PD `default-shard-count: 3` (12 partitions × 3 replicas, every partition on every store), 20 M edges, PR head `54d4fe56` vs master `60c8803`. Scripts and logs in the same place ([`results/issue-3162/`](https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3162), section "Replication 3").
| step | PR head `54d4fe56` | master `60c8803` |
|---|---|---|
| snapshot 300 ms into the leader's compaction | leader: `snapshot save failed: compaction in progress` (EBUSY), no directory; **both followers run their own `doSnapshot` and save fine** — the snapshot command is a replicated raft task, every replica snapshots its own state machine | leader commits `snapshot_50213` with `__raft_snapshot_meta` and no `data/`; the followers save fine |
| `SIGKILL` the leader's store 4 s later, restart without wiping | n/a, nothing to corrupt | `onSnapshotLoad failed` → `Raft 0 is restarting !!!` once; partition 0 on that node is dead (`/v1/partition/0` empty); leadership moved to another node |
| the cluster with one dead replica | | `g.E().count()` = 20 000 000, complete: the two healthy replicas serve |
| does the dead replica heal itself? | | **no**: its raft node never initialises, so it never asks the leader for a snapshot; the `is restarting` loop ends after one attempt |
| operator recovery: `rm -rf raft/00000 db/00000` on the dead node only, restart | | the node joins with `term=0, index=0`, the leader sends `InstallSnapshotRequest` (`lastIncludedLogIndex=50213`), follower `PState_Normal` within a minute, count still 20 000 000 |
Conclusion: with three replicas the master bug costs one replica and a manual wipe of one partition on one node, not data; with one replica (the run of Sep 12) it costs the partition and, after the only possible cleanup, the unflushed writes too. The save-side fix in this PR removes the cause in both cases, so after this measurement I see nothing open from my side. The "wipe one partition on one node → InstallSnapshot from the leader" procedure may deserve a sentence in the operations docs, because nothing in the log suggests that this is the way out.
One observation outside the scope of this PR, recorded on our side as a separate finding: when the lab's disk filled up during the run, a single-node PD stayed leaderless for good after one failed 5-minute snapshot of its own raft (`Error code = 100` on every call, stores could not register) until it was restarted. If that is of interest to the maintainers, I will file it separately.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5699551607
@imbajin @bitflicker64
Please hold off on your reviews for now. I’m making a few more improvements to the cleanPartition() functionality, especially around the range-lock.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3896232921
##########
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:
I have removed `test-snapshot-corruption.sh` because I was unable to reproduce the original issue (the `saveSnapshot` race condition) using it. Instead, I have covered those scenarios with unit tests in a more deterministic way.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3813408824
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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"
Review Comment:
Thanks @imbajin
I have addressed it.
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/usr/bin/env bash
Review Comment:
Thanks @imbajin
I have addressed it.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3818659311
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,311 @@
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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) — tested in default mode:
+# onSnapshotSave returns early → neither data/ nor should_not_load written.
+# Snapshot dir has only __raft_snapshot_meta.
+# On load: goes straight to loadSnapshot(missingDir) → "not exists" → stuck.
+# Fix 1 (throw instead of return) prevents this snapshot from ever being committed.
+#
+# 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 '/')
+SNAP_NAME=$(basename "$TARGET")
+
+if $FIXED_MODE; then
+ # Sub-case B: remove only data/, keep should_not_load.
+ # Buggy image: shouldNotLoad() fires, silently returns — no error logged.
+ # Fixed image (Fix 2): detects data/ missing, logs warning, falls through.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ only — keeping should_not_load (sub-case B)"
+ docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c "rm -rf '${TARGET}/data'
+ echo 'Contents after corruption:'; ls '${TARGET}'"
+else
+ # Sub-case A: remove both data/ and should_not_load — exactly what the race produces.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ and should_not_load — leaving only __raft_snapshot_meta (sub-case A)"
+ docker run --rm \
Review Comment:
Hi @imbajin
You're correct. The save-side throw (SnapshotHandler.java:98–101) only fires if compaction is actually in flight when snapshot() is called, and we have no reliable way to hold that state long enough. Because /test/compact submits a background task and returns immediately, the race window is too narrow to hit deterministically. This would make the test flaky.
Currently, the script simulates the outcome of that race (a missing data/ dir) by removing it manually, which lets us at least verify the load-side behavior. I have added a comment in the script to make this clear.
If there is a way to deterministically reproduce the save-side throw, I would be happy to incorporate it into the script.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3988514994
##########
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:
Addressed Review comments.
##########
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:
Addressed Review comments.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
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)
+@Suite.SuiteClasses({
+ 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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4028447564
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandler.java:
##########
@@ -230,6 +230,14 @@ void lock(String path) throws InterruptedException,
void unlock(String path);
+ /**
+ * Non-blocking attempt to reserve the compactRange() window for partition {@code id}.
+ * Returns false if a compaction is actively running for that partition right now.
+ */
+ boolean tryLockCompactionRange(int id);
Review Comment:
⚠️ Important. This PR adds three non-default abstract methods to the public `BusinessHandler` interface (`tryLockCompactionRange`, `unlockCompactionRange`, and `getPathLockState`). Any downstream plugin or test double that implements the published `hg-store-core` interface will fail to compile when upgrading to this head, even though the only in-repository implementation is updated. Requested change: keep these compaction-lock helpers behind an internal collaborator, or provide a compatibility-preserving API shape instead of expanding the public interface with mandatory methods.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/raft/PartitionStateMachine.java:
##########
@@ -198,7 +198,13 @@ public void onSnapshotSave(final SnapshotWriter writer, final Closure done) {
done.run(Status.OK());
} catch (HgStoreException e) {
log.error(String.format("Raft %s onSnapshotSave failed. {}", groupId), e);
- done.run(new Status(RaftError.EIO, e.toString()));
+ // A busy compaction-range lock is transient: jRaft's snapshot scheduler
+ // retries independently, so report EBUSY rather than EIO to avoid
+ // escalating to reportError()/restartRaftNode() (see SnapshotExecutorImpl
+ // #onSnapshotSaveDone, which only escalates on EIO).
+ RaftError raftError = e.getCode() == HgStoreException.EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL ?
Review Comment:
⚠️ Important. The new `EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL` → `RaftError.EBUSY` mapping is not tested at the state-machine boundary. The added tests assert the exception from `SnapshotHandler`, but no test invokes `PartitionStateMachine.onSnapshotSave()` and captures the `Closure` status; a regression to mapping busy failures to `EIO` would still pass while causing jRaft to escalate to `restartRaftNode()`. Please add direct coverage for busy → `EBUSY`, ordinary save failure → `EIO`, and the final lock release.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3896238294
##########
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:
Addressed.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5716035292
Thanks @imbajin @bitflicker64 for taking the time to review this PR. Special thanks to @SebastianGruza for helping hugegraph_validation related work.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4014715233
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -138,6 +143,12 @@ public class BusinessHandlerImpl implements BusinessHandler {
private static final ConcurrentMap<String, AtomicInteger> pathLock = new ConcurrentHashMap<>();
private static final ConcurrentMap<Integer, AtomicInteger> compactionState =
new ConcurrentHashMap<>();
+ // Guards the compactRange() window specifically, so a snapshot save can atomically
+ // check-and-reserve against a compaction that is actually running right now. This is
+ // narrower than pathLock, which stays held through the post-compaction blank-task
+ // snapshot and must not be reused here to avoid deadlocking that flow.
+ private static final ConcurrentMap<Integer, ReentrantLock> compactionRangeLock =
Review Comment:
⚠️ This lock is described as guarding the compactRange() window, but `cleanPartition` still compacts without it.
`private boolean cleanPartition(Partition, Function)` ends with an unguarded `op.compactRange()` (line 1290 at this head) on the same partition's RocksDB session. It is reachable from `DataManagerImpl.java:246` and `DefaultDataMover.java:245` (cleanup after split/move), `CleanTask.java:63,68`, `PartitionAPI.java:184` and `HgStoreWrapperEx.java:50`. None of them take `compactionRangeLock`, so `onSnapshotSave` can still reserve the lock and checkpoint while a full range compaction is running on that partition. The `tryLockCompactionRange` contract ("returns false if a compaction is actively running") does not hold for this path. It is the same kind of gap as the table-specific branch at line 1441, at a different call site.
Requested change: run this `compactRange()` under the same per-partition range lock and bounded wait. A single helper used by every compaction entry point would cover this site and the `compactRange(tableName)` branch together.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3896218859
##########
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:
Hi @bitflicker64 ,
Thank you for your valuable feedback. You have raised a valid concern.
But I believe "immediate throw" is the correct approach.
With the fix:
Snapshot during compaction → throw → RaftError.EIO → snapshot is not committed.
Compaction finishes → SYNC_BLANK_TASK triggers the next valid snapshot automatically.
SYNC_BLANK_TASK is submitted after compaction and, when applied, doSnapshotSync() proceeds only when the state is compactionDone. This shows the system already anticipates compaction/snapshot overlap and has a deterministic recovery path.
Waiting for compaction instead would block shared thread-pool resources for potentially 5–15 minutes and is timing-dependent. The temporary log growth from an immediate failure is bounded by the compaction duration and is preferable to the original silent corruption.
Since the probability of race condition in less, the immediate throw is better trade-off. It prevents silent corruption, leverages the existing recovery mechanism, avoids thread starvation, and requires no additional timing/configuration.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3903582547
##########
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:
⚠️ Blocking: yes. This guard is a non-atomic check followed by the save at line 105. `dbCompaction` runs asynchronously, initializes `compactionState` at `BusinessHandlerImpl.java:1414`, then sets `doing` at `1418` before `compactRange()` at `1420`; it does not synchronize with this check. If compaction starts after `getState()` returns (including while the state entry is still null), this path still calls `saveSnapshot` during compaction, so the change does not guarantee that overlapping snapshots are rejected. Please coordinate the state transition and snapshot save with the same per-partition lock or another atomic protocol, or re-check the state under such a lock immediately before saving. Evidence: exact head `SnapshotHandler.java:97-105` and `BusinessHandlerImpl.java:1408-1421`.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5342854247
Thanks @imbajin for review. I have addressed the review comments.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4014467837
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1438,42 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
Review Comment:
⚠️ Blocking: yes. This new lock acquisition is inside the tableName.isEmpty() branch. For a PD DB_COMPACTION request with a concrete tableName, the else path still invokes op.compactRange(tableName) without compactionRangeLock, while SnapshotHandler.onSnapshotSave reserves that lock. A table-specific RocksDB compaction can therefore overlap checkpoint creation and recreate the incomplete/corrupt snapshot race. Please acquire the same bounded range lock for both overloads, or explicitly prove table compaction cannot overlap snapshot creation.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3940031806
##########
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:
Addressed. Thanks @bitflicker64
##########
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:
Addressed. Thanks @bitflicker64
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3988513427
##########
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:
Addressed Review comments.
##########
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:
Addressed Review comments.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3873395656
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,311 @@
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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) — tested in default mode:
+# onSnapshotSave returns early → neither data/ nor should_not_load written.
+# Snapshot dir has only __raft_snapshot_meta.
+# On load: goes straight to loadSnapshot(missingDir) → "not exists" → stuck.
+# Fix 1 (throw instead of return) prevents this snapshot from ever being committed.
+#
+# 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 '/')
+SNAP_NAME=$(basename "$TARGET")
+
+if $FIXED_MODE; then
+ # Sub-case B: remove only data/, keep should_not_load.
+ # Buggy image: shouldNotLoad() fires, silently returns — no error logged.
+ # Fixed image (Fix 2): detects data/ missing, logs warning, falls through.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ only — keeping should_not_load (sub-case B)"
+ docker run --rm \
+ -v "${VOLUME_PREFIX}_hg-store0-data:/hugegraph-store/storage" \
+ busybox sh -c "rm -rf '${TARGET}/data'
+ echo 'Contents after corruption:'; ls '${TARGET}'"
+else
+ # Sub-case A: remove both data/ and should_not_load — exactly what the race produces.
+ log " Target: partition $PARTITION_ID / $SNAP_NAME"
+ log " Removing data/ and should_not_load — leaving only __raft_snapshot_meta (sub-case A)"
+ docker run --rm \
Review Comment:
@imbajin
I could not found deterministic steps to repro this issue ( because its timing issue). I had encountered this while testing changes related to Cloud storage support feature.
But this PR addresses critical edge case. For users running distributed HugeGraph clusters, this enhancement prevents silent failures or data inconsistency during high-throughput operations when snapshots and compactions overlap.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3813411247
##########
docker/test/test-snapshot-corruption.sh:
##########
@@ -0,0 +1,314 @@
+
+#!/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 — deterministic reproducer for the HStore snapshot corruption bug
+#
+# 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 bug is present (buggy image)
+# bash docker/hbase/test/test-snapshot-corruption.sh --fixed # confirm bug is absent (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; }
+
+# In --fixed mode use the locally-built patched image.
+# Build it from source if it doesn't exist yet so the caller only needs --fixed.
+PATCHED_IMAGE="hugegraph/store:patched"
+DOCKERFILE="$SCRIPT_DIR/../../Dockerfile.store-patched"
Review Comment:
Thanks @imbajin
I have addressed it.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#issuecomment-5344209848
## [Codecov](https://app.codecov.io/gh/apache/hugegraph/pull/3164?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 93.25%. Comparing base ([`431f6e6`](https://app.codecov.io/gh/apache/hugegraph/commit/431f6e6b431014feb5b50d27ea67180b4610f10b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f2b7219`](https://app.codecov.io/gh/apache/hugegraph/commit/f2b7219fb33c256726963b12a7eec3a9f5b48c0e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3164 +/- ##
=============================================
+ Coverage 41.06% 93.25% +52.19%
+ Complexity 519 65 -454
=============================================
Files 771 9 -762
Lines 65962 267 -65695
Branches 8766 22 -8744
=============================================
- Hits 27088 249 -26839
+ Misses 36008 8 -36000
+ Partials 2866 10 -2856
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hugegraph/pull/3164?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3990841084
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1422,27 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ if (!rangeLock.tryLock()) {
Review Comment:
⚠️ A full compaction that starts while a snapshot save holds this lock is dropped, and the event-driven callers never retry it.
The comment says the next scheduled or triggered compaction will retry, but these requests are one-shot: `handleCleanOp` after a data clean (`PartitionEngine.java:1017`), `destroyPartition` when graphs remain (`HgStoreEngine.java:505`), the PD `DB_COMPACTION` instruction (`PartitionEngine.java:1263`), and the `/compat` REST call (`PartitionAPI.java:207`), which has already told the operator the compaction was submitted. The only periodic caller is the daily TTL cleaner, and it compacts only after it cleaned something. If a periodic snapshot on the same partition is between `tryLockCompactionRange` and `unlockCompactionRange` (`SnapshotHandler.java:96-129`: checkpoint plus the checksum pass over every file), the request leaves one INFO line. The cleaned range stays uncompacted and the post-compaction `SYNC_BLANK_TASK` snapshot never runs. Before this change a snapshot never cancelled a compaction.
Requested change: the snapshot side already fails fast, so this side can wait. Use `rangeLock.tryLock(timeout, unit)` with a bound (this thread already waits up to `timeoutMillis` in `lock(path)` above), and skip with a WARN only if the save is still running when it expires. That cannot deadlock: `onSnapshotSave` only calls non-blocking `tryLock()` and never takes `pathLock`. Please also fix the comment.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r3995820616
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1422,27 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ if (!rangeLock.tryLock()) {
Review Comment:
Thanks @bitflicker64. I have implemented the suggested changed.
I kept compactionRangeLockWaitMillis = 10_000. I don't think config to adjust compactionRangeLockWaitMillis is necessary.
saveSnapshot() should be very fast, so the lock is normally held for well under a second, and at most a few seconds even under slow or heavily loaded disk conditions. A 10-second timeout provides a generous margin over the expected lock-hold time while still preventing the lock from being held indefinitely.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "vaijosh (via GitHub)" <gi...@apache.org>.
vaijosh commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4024554835
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1415,10 +1438,42 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
log.info("Partition {} dbCompaction started", id);
if (tableName.isEmpty()) {
lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
+ ReentrantLock rangeLock =
Review Comment:
Thanks @imbajin. I have fixed this in recent commit.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(store): throw on compaction-busy snapshot save; validate data/ on load (#3162) [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4027644539
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1413,30 +1456,58 @@ public boolean dbCompaction(String graphName, int id, String tableName) {
pathLock.putIfAbsent(path, new AtomicInteger(compactionCanStart));
compactionState.putIfAbsent(id, new AtomicInteger(0));
log.info("Partition {} dbCompaction started", id);
- if (tableName.isEmpty()) {
- lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction start", id, path);
- op.compactRange();
- setState(id, compactionDone);
- log.info("Partition {} dbCompaction end and start to do snapshot", id);
- PartitionEngine pe = HgStoreEngine.getInstance().getPartitionEngine(id);
- // find leader and send blankTask, after execution
- if (pe.isLeader()) {
- RaftClosure bc = (closure) -> {
- };
- pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
- bc);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked = rangeLock.tryLock(compactionRangeLockWaitMillis,
+ TimeUnit.MILLISECONDS);
+ if (rangeLocked) {
+ if (tableName.isEmpty()) {
+ lock(path);
+ setState(id, doing);
+ log.info("Partition {}-{} got lock, dbCompaction start", id, path);
+ op.compactRange();
+ setState(id, compactionDone);
+ log.info("Partition {} dbCompaction end and start to do snapshot", id);
+ PartitionEngine pe = HgStoreEngine.getInstance().getPartitionEngine(id);
+ // find leader and send blankTask, after execution
+ if (pe.isLeader()) {
+ RaftClosure bc = (closure) -> {
+ };
+ pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
+ bc);
+ } else {
+ HgCmdClient client = HgStoreEngine.getInstance().getHgCmdClient();
+ BlankTaskRequest request = new BlankTaskRequest();
+ request.setGraphName("");
+ request.setPartitionId(id);
+ client.tryInternalCallSyncWithRpc(request);
+ }
+ setAndNotifyState(id, compactionDone);
+ } else {
+ op.compactRange(tableName);
+ }
} else {
- HgCmdClient client = HgStoreEngine.getInstance().getHgCmdClient();
- BlankTaskRequest request = new BlankTaskRequest();
- request.setGraphName("");
- request.setPartitionId(id);
- client.tryInternalCallSyncWithRpc(request);
+ log.warn("Partition {} skip dbCompaction, snapshot save " +
+ "still in progress after {}ms wait", id,
+ compactionRangeLockWaitMillis);
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ log.warn("Partition {} dbCompaction interrupted while waiting " +
+ "for snapshot range lock", id);
+ // Interrupted while waiting for the snapshot save to release
+ // the range lock. The path lock was already acquired and
+ // must be released here, otherwise later compactions for this
+ // partition would block until the path lock timeout.
+ unlock(path);
Review Comment:
⚠️ Important. The last commit moved `rangeLock.tryLock(...)` in front of `lock(path)` (in 54d4fe5 it came after). That causes two problems.
1. This `unlock(path)` now runs when the task never held the path lock: an interrupt in `tryLock` at 1464, or in `lock(path)` itself, or any interrupt on the `tableName` branch, which never takes it at all. `unlock` sets the shared `AtomicInteger` back to `compactionCanStart`. If an earlier compaction on this partition still holds it (it stays held until `PartitionEngine.doSnapshotSync` finishes), that lock gets cleared and a third compaction can start before the earlier post-compaction snapshot is done. The comment above says "the path lock was already acquired", which is no longer true. `testDbCompactionReleasesPathLockWhenInterruptedWaitingForRangeLock` still passes only because the path lock was never set to `doing` in that test.
2. The range lock is now held while `lock(path)` waits, which can be up to `timeoutMillis` (6h), and while the blank-task RPC runs. When a second `dbCompaction` for the same partition arrives while the first one's post-compaction snapshot is pending (the callers at PartitionEngine.java:1018/1266, HgStoreEngine.java:505 and the TTL submitter don't go through the semaphore), it takes the range lock and blocks in `lock(path)`. The snapshot that would release `path` then calls `tryLockCompactionRange`, gets EBUSY, and every periodic snapshot on the partition fails the same way until the wait ends.
Requested change: go back to taking `lock(path)` first, hold the range lock only around `op.compactRange()` / `op.compactRange(tableName)`, and call `unlock(path)` only when `lock(path)` actually succeeded (for example with a `pathLocked` flag). Please also update the test so the path lock is really in `doing` before the interrupt.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1190,15 +1212,36 @@ public boolean cleanPartition(String graph, int partId, long startKey, long endK
taskManager.putAsyncTask(cleanTask);
Utils.runInThread(() -> {
- cleanPartition(partition, code -> {
- // in range
- boolean flag = code >= startKey && code < endKey;
- return (cleanType == CleanType.CLEAN_TYPE_KEEP_RANGE) == flag;
- });
- // May have been destroyed.
- if (HgStoreEngine.getInstance().getPartitionEngine(partId) != null) {
- taskManager.updateAsyncTaskState(partId, graph, cleanTask.getId(),
- AsyncTaskState.SUCCESS);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(partId, k -> new ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked = rangeLock.tryLock(compactionRangeLockWaitMillis,
Review Comment:
⚠️ Important. This lock is now held for the entire `cleanPartition(partition, ...)` call. That call scans every key in the partition and deletes the ones out of range before it reaches `op.compactRange()` at line 1310. On a large partition that can take minutes. For all of that time every `onSnapshotSave` for the partition fails with EBUSY, so the raft log isn't truncated, and any `dbCompaction` on the partition waits 10s and is then dropped. The field comment at 168-178 assumes the lock is held for under a second.
The skip branch also has a cost: if a snapshot save holds the lock past 10s, the whole cleanup is dropped, not just the compaction. This is the one-shot cleanup after a split or move (DataManagerImpl.java:246, DefaultDataMover.java:245), so the data that no longer belongs to this partition stays, and the CleanTask is left in START.
Requested change: take the range lock only around the final `op.compactRange()` inside the private `cleanPartition(Partition, Function)`, with the same bounded wait, and let the delete pass run unlocked. If the compaction step times out, log it but keep the cleanup.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org