bitflicker64 commented on code in PR #3134:
URL: https://github.com/apache/hugegraph/pull/3134#discussion_r4234584971
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStdSessions.java:
##########
@@ -334,6 +339,13 @@ private void checkValid() {
E.checkState(this.rocksdb.isOwningHandle(), "It seems RocksDB has been
closed");
}
+ private void replaceSeparateWalDirectory() throws IOException {
Review Comment:
Important: should not exist. `replaceSeparateWalDirectory` is private and
nothing in production calls it: `git grep -n replaceSeparateWalDirectory
90d5775` finds only this definition and three
`Whitebox.invoke(RocksDBStdSessions.class, "replaceSeparateWalDirectory",
sessions)` calls in `RocksDBSessionsTest`
(`testSeparateWalInstallsCheckpointTail`,
`testDataInsideWalKeepsDataAndInstallsTail`, `testWalInsideDataInstallsTail`).
Production installs the WAL through `RocksDBSnapshotRestore.install()`.
Delete the method. Those three tests are really testing
`RocksDBSnapshotRestore.installWal`, which is package-private. Move them into
`RocksDBSnapshotRestoreTest`, which is already in that package, and call
`RocksDBSnapshotRestore.installWal(data, wal, new
RocksDBSnapshotRestore.FileOperations())` directly. If the existing
`testResumeWhen*Inside*` resume tests already cover the nested cases, drop them
instead.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/preload-topling.sh:
##########
@@ -0,0 +1,230 @@
+#!/bin/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.
+#
+
+ORIG_SHELL_FLAGS="$-"
+ORIG_PIPEFAIL="$(set -o | awk '$1 == "pipefail" { print $2 }')"
+ORIG_ERR_TRAP="$(trap -p ERR)"
+# Save original IFS to avoid leaking into parent shell when sourced
+ORIG_IFS="${IFS}"
+set -Eeuo pipefail
+IFS=$'\n\t'
+# Unified error capture for easy positioning
+trap 'echo "[preload-topling] error at line ${LINENO}: ${BASH_COMMAND}" >&2'
ERR
+
+RUNTIME_BIN="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+COMPONENT_TOP="$(cd "$RUNTIME_BIN"/../ && pwd)"
+COMPONENT_LIB="$COMPONENT_TOP/lib"
+DEST_DIR="$COMPONENT_TOP/library"
+
+detect_rocksdb_provider() {
+ local conf_dir="$1"
+ local file value provider="" seen_provider=false
+
+ while IFS= read -r value; do
+ if [ "$seen_provider" = false ]; then
+ provider="$value"
+ seen_provider=true
+ elif [ "$provider" != "$value" ]; then
+ echo "Error: conflicting rocksdb.provider values:
$provider,$value" >&2
+ return 1
+ fi
+ done < <(
+ for file in "$conf_dir"/graphs/*.properties; do
+ [ -f "$file" ] || continue
+ awk '
+ /^[[:space:]]*#/ { next }
+ /^[[:space:]]*rocksdb\.provider[[:space:]]*=/ {
+ value = $0
+ sub(/^[^=]*=[[:space:]]*/, "", value)
+ sub(/[[:space:]]+#.*/, "", value)
+ gsub(/^[[:space:]]+|[[:space:]]+$/, "", value)
+ print value
+ }
+ ' "$file"
+ done
+ for file in "$conf_dir"/application*.yml; do
+ [ -f "$file" ] || continue
+ awk '
+ /^[[:space:]]*#/ || /^[[:space:]]*$/ { next }
+ /^[^[:space:]#][^:]*:/ {
+ in_rocksdb = ($0 ~ /^rocksdb[[:space:]]*:/)
+ }
+ in_rocksdb && /^[[:space:]]+provider[[:space:]]*:/ {
+ value = $0
+ sub(/^[^:]*:[[:space:]]*/, "", value)
+ sub(/[[:space:]]+#.*/, "", value)
+ gsub(/^[[:space:]]+|[[:space:]]+$/, "", value)
+ if (value ~ /^"[^"]*"$/ || value ~ /^'\''[^'\'']*'\''$/) {
+ value = substr(value, 2, length(value) - 2)
+ }
+ print value
+ }
+ ' "$file"
+ done
+ )
+
+ provider="${provider:-rocksdb}"
+ case "$provider" in
+ rocksdb | topling)
+ echo "$provider"
+ ;;
+ *)
+ echo "Error: invalid rocksdb.provider '$provider'; expected
rocksdb or topling" >&2
+ return 1
+ ;;
+ esac
+}
+
+SERVER_CONFIG_ACTIVE=false
+if [ -f "$RUNTIME_BIN/rocksdb-server-config.sh" ] ||
+ [ -f "${REST_SERVER_CONF:-$COMPONENT_TOP/conf/rest-server.properties}" ] ||
+ [ -d "$COMPONENT_TOP/conf/graphs" ]; then
+ SERVER_CONFIG_ACTIVE=true
+ source "$RUNTIME_BIN/rocksdb-server-config.sh"
+
SERVER_REST_CONFIG="${REST_SERVER_CONF:-$COMPONENT_TOP/conf/rest-server.properties}"
+ [[ "$SERVER_REST_CONFIG" = /* ]] ||
SERVER_REST_CONFIG="$COMPONENT_TOP/$SERVER_REST_CONFIG"
+ SERVER_GRAPHS_DIR=$(server_graphs_directory "$COMPONENT_TOP"
"$SERVER_REST_CONFIG") || exit 1
+ PROVIDER=$(server_rocksdb_provider "$SERVER_GRAPHS_DIR") || exit 1
+ server_check_provider_override "$PROVIDER" || exit 1
+else
+ PROVIDER="${TOPLINGDB_ROCKSDB_PROVIDER:-}"
+ if [ -n "$PROVIDER" ]; then
+ case "$PROVIDER" in
+ rocksdb | topling) ;;
+ *)
+ echo "Error: invalid TOPLINGDB_ROCKSDB_PROVIDER '$PROVIDER';" \
+ "expected rocksdb or topling" >&2
+ exit 1
+ ;;
+ esac
+ else
+ PROVIDER=$(detect_rocksdb_provider "$COMPONENT_TOP/conf") || exit 1
+ fi
+fi
+
+remove_path_entry() {
+ local value="${1:-}"
+ local remove="${2:-}"
+ local entry result=""
+ local path_ifs="$IFS"
+ IFS=:
+ for entry in $value; do
+ [ -n "$entry" ] && [ "$entry" != "$remove" ] || continue
+ result="${result:+$result:}$entry"
+ done
+ IFS="$path_ifs"
+ echo "$result"
+}
+
+# A parent launcher may start multiple components from one shell. Remove only
+# the runtime entry previously selected by this helper before selecting ours.
+if [ -n "${TOPLING_ACTIVE_NATIVE:-}" ]; then
+ LD_PRELOAD=$(remove_path_entry "${LD_PRELOAD:-}" "$TOPLING_ACTIVE_NATIVE")
+ LD_LIBRARY_PATH=$(remove_path_entry "${LD_LIBRARY_PATH:-}" \
+ "$(dirname "$TOPLING_ACTIVE_NATIVE")")
+ export LD_PRELOAD LD_LIBRARY_PATH
+fi
+if [ -n "${TOPLING_ACTIVE_JAR:-}" ]; then
+ CLASSPATH=$(remove_path_entry "${CLASSPATH:-}" "$TOPLING_ACTIVE_JAR")
+ export CLASSPATH
+fi
+unset TOPLING_ACTIVE_NATIVE TOPLING_ACTIVE_JAR TOPLING_RUNTIME_CLASSPATH
+
+if [ "$PROVIDER" = "topling" ]; then
+ # Runtime binaries are read-only; installation prepares them beforehand.
+ if [ "$(uname -s)" != "Linux" ] || [ "$(uname -m)" != "x86_64" ]; then
+ echo "Error: ToplingDB runtime supports Linux x86_64 only" >&2
+ exit 1
+ fi
+ if [ "$SERVER_CONFIG_ACTIVE" = true ]; then
+ server_verify_graph_roots "$COMPONENT_TOP" "$SERVER_GRAPHS_DIR"
"$PROVIDER" \
+ "${HG_SERVER_ENFORCE_PROVIDER_MARKER:-false}" || exit 1
+ fi
+ TOPLING_JAR=$(ls -1 "$COMPONENT_LIB"/topling/rocksdbjni*.jar 2>/dev/null |
+ sort -V | tail -1 || true)
+ if [ -z "$TOPLING_JAR" ]; then
+ echo "Error: no prepared ToplingDB JAR found in
$COMPONENT_LIB/topling/" >&2
+ exit 1
+ fi
+
+ CONF_FILE="${TOPLINGDB_EASY_MIGRATE_CONF:-}"
+ if [ -z "$CONF_FILE" ]; then
+ CONF_FILE="$COMPONENT_TOP/conf/toplingdb.yaml"
+ if [ ! -f "$CONF_FILE" ]; then
+ CONF_FILE="$COMPONENT_TOP/conf/rocksdb_store.yaml"
+ fi
+ if [ ! -f "$CONF_FILE" ]; then
+ CONF_FILE="$COMPONENT_TOP/conf/rocksdb_pd.yaml"
+ fi
+ fi
+ if [ ! -f "$CONF_FILE" ]; then
+ echo "Error: required ToplingDB Easy Migrate config not found:
$CONF_FILE" >&2
+ exit 1
+ fi
+ if [ ! -r "$CONF_FILE" ]; then
+ echo "Error: ToplingDB Easy Migrate config is not readable:
$CONF_FILE" >&2
+ exit 1
+ fi
+ export TOPLINGDB_EASY_MIGRATE_CONF="$CONF_FILE"
+ echo "[preload-topling] TOPLINGDB_EASY_MIGRATE_CONF=$CONF_FILE"
+ NATIVE_LIBRARY="$DEST_DIR/librocksdbjni-linux64.so"
+ if [ ! -r "$NATIVE_LIBRARY" ]; then
+ echo "Error: prepared ToplingDB native library not found:
$NATIVE_LIBRARY" >&2
+ echo " Run bin/prepare-topling.sh for this component before
startup." >&2
+ exit 1
+ fi
+ export LD_LIBRARY_PATH="$DEST_DIR${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
+ if command -v ldd >/dev/null 2>&1 &&
+ ldd "$NATIVE_LIBRARY" 2>/dev/null | grep -q 'not found'; then
+ echo "Error: ToplingDB native library has unresolved system
dependencies" >&2
+ echo " Install them during package/deployment preparation." >&2
+ exit 1
+ fi
+
+ export LD_PRELOAD="${LD_PRELOAD:+$LD_PRELOAD:}$NATIVE_LIBRARY"
+ export TOPLING_ACTIVE_NATIVE="$NATIVE_LIBRARY"
+ export TOPLING_RUNTIME_CLASSPATH="$TOPLING_JAR"
+ export CLASSPATH="$TOPLING_JAR${CLASSPATH:+:$CLASSPATH}"
+ export TOPLING_ACTIVE_JAR="$TOPLING_JAR"
+else
+ if [ "$SERVER_CONFIG_ACTIVE" = true ]; then
+ server_verify_graph_roots "$COMPONENT_TOP" "$SERVER_GRAPHS_DIR"
"$PROVIDER" \
Review Comment:
Important: should not exist. This is the `provider=rocksdb` branch, so it
runs on every stock tarball start (`hugegraph.properties:24` is
`backend=rocksdb`, provider commented out). On that path
`server_verify_graph_roots` does three things the default provider never needed:
- `server_verify_db_mounts` (`rocksdb-server-config.sh:274-326`) refuses to
start on Linux unless `mountpoint` is util-linux 2.37+. Ubuntu 20.04 ships
2.34, RHEL 8 ships 2.32. Java already does the same bind-mount check in
`RocksDBSnapshotRestore.lock()` via `isLinuxMountPoint`.
- `server_claim_standard_path` (`:213-240`) writes a
`.hugegraph-rocksdb-provider` file into every new standard data and WAL
directory, to win a race against a concurrent Topling startup on the same path.
That race is speculative, and the Java `.resume-lock` already serializes
cross-process opens.
- the `rocksdb.data_disks` rejection imbajin already reported.
The one job this branch has is refusing to open Topling-marked data as plain
RocksDB, and `server_check_existing_markers` (`:170-208`) already does that
read-only and works on macOS. Replace the call with:
```bash
for f in "$SERVER_GRAPHS_DIR"/*.properties; do
case "$(server_property "$f" backend memory)" in rocksdb | rocksdbsst)
;; *) continue ;; esac
for key in data wal; do
p=$(server_property "$f" "rocksdb.${key}_path" "rocksdb-data/$key")
|| exit 1
server_check_existing_markers "$(server_storage_path
"$COMPONENT_TOP" "$p")" rocksdb || exit 1
done
done
```
Then `server_claim_standard_path` and the `rocksdb`/`false` branch in
`server_verify_storage_path` have no callers and can go. Keep the full
`server_verify_graph_roots` on the Topling branch at line 155.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStdSessions.java:
##########
@@ -365,61 +377,139 @@ private void ingestExternalFile() throws
RocksDBException {
}
}
+ private static void finishRestore(RocksDBSnapshotRestore restore,
OpenedRocksDB opened) {
+ if (restore != null) {
+ try {
+ restore.complete();
+ } catch (RuntimeException e) {
+ opened.close();
+ throw e;
+ }
+ }
+ }
+
private static OpenedRocksDB openRocksDB(HugeConfig config, String
dataPath,
- String walPath) throws
RocksDBException {
- // Init options
- Options options = new Options();
- RocksDBStdSessions.initOptions(config, options, options, options,
options);
- options.setWalDir(walPath);
- SstFileManager sstFileManager = new SstFileManager(Env.getDefault());
- options.setSstFileManager(sstFileManager);
- /*
- * Open RocksDB at the first time
- * Don't merge old CFs, we expect a clear DB when using this one
- */
- RocksDB rocksdb = RocksDB.open(options, dataPath);
- Map<String, OpenedRocksDB.CFHandle> cfs = new ConcurrentHashMap<>();
- return new OpenedRocksDB(rocksdb, cfs, sstFileManager);
+ String walPath) throws
+ RocksDBException {
+ FileChannel recoveryLock = lockForOpen(dataPath);
+ try {
+ RocksDBSnapshotRestore restore =
RocksDBSnapshotRestore.prepareOpen(dataPath, walPath);
+ // Init options
+ Options options = new Options();
+ RocksDBStdSessions.initOptions(config, options, options, options,
options);
+ options.setWalDir(walPath);
+ SstFileManager sstFileManager = new
SstFileManager(Env.getDefault());
+ options.setSstFileManager(sstFileManager);
+
+ RocksDB rocksdb = RocksDB.open(options, dataPath);
+
+ Map<String, OpenedRocksDB.CFHandle> cfs = new
ConcurrentHashMap<>();
+ OpenedRocksDB opened = new OpenedRocksDB(rocksdb, cfs,
sstFileManager, recoveryLock);
+ finishRestore(restore, opened);
+ return opened;
+ } catch (RuntimeException | RocksDBException | Error e) {
+ RocksDBSnapshotRestore.unlock(recoveryLock);
+ throw e;
+ }
}
private static OpenedRocksDB openRocksDB(HugeConfig config,
List<String> cfNames, String
dataPath,
- String walPath) throws
RocksDBException {
- // Old CFs should always be opened
- Set<String> mergedCFs = RocksDBStdSessions.mergeOldCFs(dataPath,
- cfNames);
- List<String> cfs = ImmutableList.copyOf(mergedCFs);
-
- // Init CFs options
- List<ColumnFamilyDescriptor> cfds = new ArrayList<>(cfs.size());
- for (String cf : cfs) {
- ColumnFamilyDescriptor cfd = new
ColumnFamilyDescriptor(encode(cf));
- ColumnFamilyOptions options = cfd.getOptions();
- RocksDBStdSessions.initOptions(config, null, null, options,
options);
- cfds.add(cfd);
+ String walPath) throws
+ RocksDBException {
+ return openRocksDB(config, cfNames, dataPath, walPath, null);
+ }
+
+ private static OpenedRocksDB openRocksDB(HugeConfig config, List<String>
cfNames,
+ String dataPath, String walPath,
+ FileChannel heldLock) throws
RocksDBException {
+ FileChannel recoveryLock = heldLock != null ? heldLock :
lockForOpen(dataPath);
+ try {
+ RocksDBSnapshotRestore restore =
RocksDBSnapshotRestore.prepareOpen(dataPath, walPath);
+ // Old CFs should always be opened
+ Set<String> mergedCFs = RocksDBStdSessions.mergeOldCFs(dataPath,
+ cfNames);
+ List<String> cfs = ImmutableList.copyOf(mergedCFs);
+
+ // Init CFs options
+ List<ColumnFamilyDescriptor> cfds = new ArrayList<>(cfs.size());
+ for (String cf : cfs) {
+ ColumnFamilyDescriptor cfd = new
ColumnFamilyDescriptor(encode(cf));
+ ColumnFamilyOptions options = cfd.getOptions();
+ RocksDBStdSessions.initOptions(config, null, null, options,
options);
+ cfds.add(cfd);
+ }
+
+ // Init DB options
+ DBOptions options = new DBOptions();
+ RocksDBStdSessions.initOptions(config, options, options, null,
null);
+ if (walPath != null) {
+ options.setWalDir(walPath);
+ }
+ SstFileManager sstFileManager = new
SstFileManager(Env.getDefault());
+ options.setSstFileManager(sstFileManager);
+ // Open RocksDB with CFs
+ List<ColumnFamilyHandle> cfhs = new ArrayList<>();
+
+ RocksDB rocksdb = RocksDB.open(options, dataPath, cfds, cfhs);
+
+ E.checkState(cfhs.size() == cfs.size(),
+ "Expect same size of cf-handles and cf-names");
+ // Collect CF Handles
+ Map<String, OpenedRocksDB.CFHandle> cfHandles = new
ConcurrentHashMap<>();
+ for (int i = 0; i < cfs.size(); i++) {
+ cfHandles.put(cfs.get(i), new OpenedRocksDB.CFHandle(rocksdb,
cfhs.get(i)));
+ }
+ OpenedRocksDB opened = new OpenedRocksDB(rocksdb, cfHandles,
sstFileManager, recoveryLock);
+ finishRestore(restore, opened);
+ return opened;
+ } catch (RuntimeException | RocksDBException | Error e) {
+ RocksDBSnapshotRestore.unlock(recoveryLock);
+ throw e;
}
+ }
- // Init DB options
- DBOptions options = new DBOptions();
- RocksDBStdSessions.initOptions(config, options, options, null, null);
- if (walPath != null) {
- options.setWalDir(walPath);
+ private static FileChannel lockForOpen(String dataPath) throws
RocksDBException {
+ try {
+ return RocksDBSnapshotRestore.lock(dataPath);
+ } catch (BackendException e) {
+ // Opening a session must retain the checked RocksDB failure
contract,
+ // including the contention classification used by Store's
shared-CF path.
+ Throwable cause = e.getCause();
+ String message;
+ boolean contention = false;
+ if (cause instanceof OverlappingFileLockException) {
+ contention = true;
+ message = "IO error: lock hold by current process: " +
Review Comment:
Minor: local tidy. These two messages copy RocksDB's native wording (`IO
error: lock hold by current process ... No locks available`). The comment says
Store's shared-CF path needs that text, but it no longer reads it:
`RocksDBStore.java:322` now branches on `isContention()`. The only thing still
reading the text is the unchanged assertion at `MultiGraphsTest.java:401-404`.
Classification also round-trips through
`cause.getMessage().startsWith("Database is already open or recovering:")`, a
string that `RocksDBSnapshotRestore.lock()` produces a few files over.
Throw the lock's own message, `throw new
RecoveryLockException(e.getMessage() + reason, contention)`. Set `contention`
from a dedicated exception type that `lock()` throws for the `tryLock() ==
null` case, not from a message prefix. Then update `MultiGraphsTest:401-404`
the way you already updated `:416`.
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBSnapshotRestore.java:
##########
@@ -0,0 +1,400 @@
+/*
+ * 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.backend.store.rocksdb;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.channels.FileChannel;
+import java.nio.channels.OverlappingFileLockException;
+import java.nio.charset.StandardCharsets;
+import java.util.stream.Stream;
+import java.nio.file.Files;
+import java.nio.file.LinkOption;
+import java.nio.file.FileAlreadyExistsException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.StandardOpenOption;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.hugegraph.backend.BackendException;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+/**
+ * Fail-closed snapshot installation. The marker lives outside the data
directory
+ * and the original checkpoint remains the recovery source until after
reopening.
+ * A subsequent open retries the recorded checkpoint before native recovery
runs.
+ */
+final class RocksDBSnapshotRestore {
Review Comment:
Important: should not exist. I'm not asking you to drop this fail-closed
restore. It is data-loss protection, and the old delete-then-move in
`resumeSnapshot` did have a crash window. But nothing in it depends on
`rocksdb.provider`. It replaces `resumeSnapshot`, `reloadRocksDB` and every
`openRocksDB` path, and adds a sibling `.resume-lock` taken on every open for
every user. That is this 400-line class, the lock transfer in `OpenedRocksDB`,
`lockForOpen`/`RecoveryLockException`, the `RocksDBStore` write-lock and
contention changes, and a 729-line `RocksDBSnapshotRestoreTest`. The PR
description never mentions any of it.
Your own `.goal-task/toplingdb-linux-closure/pr-split-handoff.md` already
lists this as unit 3, "Snapshot/WAL safety", one that the standard provider can
verify on its own. Move that unit into its own PR against the default provider.
Reviewers can then check the recovery semantics on their own, and this PR loses
roughly 1,300 lines that have nothing to do with Topling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
