VGalaxies commented on code in PR #3024:
URL: https://github.com/apache/hugegraph/pull/3024#discussion_r3564824504
##########
hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/OpenedRocksDB.java:
##########
@@ -90,7 +91,8 @@ public void close() {
}
this.cfHandles.clear();
- this.rocksdb.close();
+ // Use RocksDBProviderLoader to close RocksDB
+ RocksDBProviderLoader.closeRocksDB(this.rocksdb);
Review Comment:
**High: Snapshot restore bypasses provider-specific cleanup**
`hugegraph-server/hugegraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/OpenedRocksDB.java:95`
**Evidence**
- Normal closure now passes through `RocksDBProviderLoader`, but
`resumeSnapshot()` calls `forceCloseRocksDB()`, which directly closes the
underlying `RocksDB` object at `RocksDBStdSessions.java:226` before reopening
through the provider.
**Impact**
- Topling’s repository mapping, native resources, and HTTP server can
survive snapshot replacement, causing leaks or a port-binding failure during
reopen.
**Requested fix**
- Make force-close close the `OpenedRocksDB` wrapper through the provider,
including column-family and repository cleanup, and test Topling snapshot
restoration with HTTP enabled.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -99,6 +99,8 @@ if [[ $PRELOAD == "true" ]]; then
sed -i -e '/registerBackends/d; /serverStarted/d'
"${SCRIPTS}/${EXAMPLE_SCRIPT}"
fi
+source $BIN/preload-topling.sh
Review Comment:
**High: Standard RocksDB startup unconditionally requires Topling
prerequisites**
`hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:102`
**Evidence**
- The default provider remains `standard`, but every startup sources the
strict preloader. That path requires `unzip` and downloads jemalloc when it is
not installed; failures terminate the sourced script. `init-store.sh:50` has
the same behavior.
**Impact**
- Minimal or offline Linux installations using standard RocksDB can no
longer initialize or start.
**Requested fix**
- Run the preload path only when the packaged engine/provider requires
Topling native preloading; leave standard RocksDB JNI loading unchanged.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/common-topling.sh:
##########
@@ -0,0 +1,308 @@
+#!/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.
+#
+set -Eeuo pipefail
+IFS=$'\n\t'
+trap 'echo "[common-topling] error at line ${LINENO}: ${BASH_COMMAND}" >&2' ERR
+
+GITHUB="https://github.com";
+
+function abs_path() {
+ local SOURCE
+ SOURCE="${BASH_SOURCE[0]}"
+ while [[ -h "$SOURCE" ]]; do
+ local DIR
+ DIR="$(cd -P "$(dirname "$SOURCE")" && pwd)"
+ SOURCE="$(readlink "$SOURCE")"
+ [[ $SOURCE != /* ]] && SOURCE="$DIR/$SOURCE"
+ done
+ cd -P "$(dirname "$SOURCE")" && pwd
+}
+
+function extract_so_with_jar() {
+ local jar_file="$1"
+ local dest_dir="$2"
+ local abs_jar_path
+
+ if [ ! -f "$jar_file" ]; then
+ echo "'$jar_file' Not Exist" >&2
+ return 1
+ fi
+
+ mkdir -p "$dest_dir" || {
+ echo "Cannot mkdir '$dest_dir'" >&2
+ return 1
+ }
+
+ if command -v realpath >/dev/null 2>&1; then
+ abs_jar_path="$(realpath "$jar_file")"
+ else
+ abs_jar_path="$(readlink -f "$jar_file")"
+ fi
+ if ! command -v unzip >/dev/null 2>&1; then
+ echo "Error: 'unzip' command not found. Please install unzip." >&2
+ return 1
+ fi
+ unzip -j -o "$abs_jar_path" "*.so" -d "$dest_dir" > /dev/null 2>&1 || {
+ local code=$?
+ if [ $code -eq 11 ]; then
+ echo "Error: No .so files found in '$abs_jar_path' (unzip exit
11)" >&2
+ else
+ echo "Error: unzip failed (exit $code) for '$abs_jar_path'" >&2
+ fi
+ return $code
+ }
+}
+
+function extract_html_css_from_jar() {
+ local jar_file="$1"
+ local dest_dir="$2"
+ local abs_jar_path
+ # Prefer /dev/shm on Linux for speed; fallback to TMPDIR or /tmp
+ local resource_target
+ if [ "$(uname -s)" = "Linux" ] && [ -d /dev/shm ]; then
+ resource_target="/dev/shm/rocksdb_resource"
+ else
+ resource_target="${TMPDIR:-/tmp}/rocksdb_resource"
+ fi
+
+ if [ ! -f "$jar_file" ]; then
+ echo "Error: JAR file '$jar_file' does not exist." >&2
+ return 1
+ fi
+
+ mkdir -p "$dest_dir" || {
+ echo "Error: Cannot create destination directory '$dest_dir'." >&2
+ return 1
+ }
+
+ if command -v realpath >/dev/null 2>&1; then
+ abs_jar_path="$(realpath "$jar_file")"
+ else
+ abs_jar_path="$(readlink -f "$jar_file")"
+ fi
+ if ! command -v unzip >/dev/null 2>&1; then
+ echo "Error: 'unzip' command not found. Please install unzip." >&2
+ return 1
+ fi
+ unzip -j -o "$abs_jar_path" "*.html" "*.css" -d "$dest_dir" > /dev/null ||
{
+ local code=$?
+ if [ $code -eq 11 ]; then
+ echo "Notice: No .html or .css files found in '$jar_file'." >&2
+ return 0
+ else
+ echo "Error: unzip failed with exit code $code" >&2
+ return $code
+ fi
+ }
+
+ mkdir -p "$resource_target" || {
+ echo "Error: Cannot create target directory '$resource_target'." >&2
+ return 1
+ }
+
+ if compgen -G "$dest_dir"/*.html >/dev/null 2>&1; then
+ cp -f "$dest_dir"/*.html "$resource_target"/
+ fi
+ if compgen -G "$dest_dir"/*.css >/dev/null 2>&1; then
+ cp -f "$dest_dir"/*.css "$resource_target"/
+ fi
+}
+
+function ensure_libaio_symlink() {
+ # Check for Ubuntu 24.04+ and create a symlink for libaio if needed.
+ # This is a workaround for software expecting the old libaio.so.1 name,
+ # as it was renamed to libaio.so.1t64 in the new release.
+ # https://askubuntu.com/questions/1512196/libaio1-on-noble/1516639#1516639
+ if [ -f /etc/os-release ]; then
+ . /etc/os-release
+ if [ "${ID:-}" = "ubuntu" ] && command -v dpkg >/dev/null 2>&1 && dpkg
--compare-versions "${VERSION_ID:-0}" "ge" "24.04"; then
+ local libaio_link_target="/usr/lib/x86_64-linux-gnu/libaio.so.1"
+ if [ ! -e "$libaio_link_target" ]; then
+ echo "Ubuntu ${VERSION_ID:-?} detected. Creating compatibility
symlink for libaio."
+ if [ -e /usr/lib/x86_64-linux-gnu/libaio.so.1t64 ]; then
+ if [ "$EUID" -eq 0 ]; then
+ ln -sf /usr/lib/x86_64-linux-gnu/libaio.so.1t64
"$libaio_link_target" || true
+ elif command -v sudo >/dev/null 2>&1; then
+ sudo ln -sf /usr/lib/x86_64-linux-gnu/libaio.so.1t64
"$libaio_link_target" || true
+ else
+ echo "Warn: sudo not available, skip creating
$libaio_link_target" >&2
+ fi
+ else
+ echo "Warn: libaio.so.1t64 not found, skip creating compat
symlink" >&2
+ fi
+ fi
+ fi
+ fi
+}
+
+function download_and_verify() {
+ local url=$1
+ local filepath=$2
+ local expected_sha256=$3
+ local actual_sha256
+
+ if [[ -f "$filepath" ]]; then
+ echo "File $filepath exists. Verifying SHA-256 checksum..."
+ actual_sha256=$(sha256sum "$filepath" | awk '{ print $1 }')
+ if [[ "$actual_sha256" != "$expected_sha256" ]]; then
+ echo "SHA-256 checksum verification failed for $filepath.
Expected: $expected_sha256, but got: $actual_sha256"
+ echo "Deleting $filepath..."
+ rm -f "$filepath"
+ else
+ echo "SHA-256 checksum verification succeeded for $filepath."
+ return 0
+ fi
+ fi
+
+ echo "Downloading $filepath..."
+ curl -fL -o "$filepath" "$url"
+
+ actual_sha256=$(sha256sum "$filepath" | awk '{ print $1 }')
+ if [[ "$actual_sha256" != "$expected_sha256" ]]; then
+ echo "SHA-256 checksum verification failed for $filepath after
download. Expected: $expected_sha256, but got: $actual_sha256"
+ return 1
+ fi
+
+ return 0
+}
+
+function download_and_setup_jemalloc() {
+ local arch lib_file download_url expected_sha256 system_lib top
+ top=$1
+
+ # Prefer system-installed jemalloc if available
+ # Try ldconfig first to locate the shared object
+ if command -v ldconfig >/dev/null 2>&1; then
+ system_lib=$(ldconfig -p 2>/dev/null | awk '/jemalloc/{print $4}' |
head -n1)
+ fi
+ # Fallback to common library paths if ldconfig is not available or found
nothing
+ if [[ -z "$system_lib" ]]; then
+ for p in \
+ /usr/lib/libjemalloc.so \
+ /usr/lib/libjemalloc.so.2 \
+ /usr/lib64/libjemalloc.so \
+ /usr/lib64/libjemalloc.so.2 \
+ /usr/local/lib/libjemalloc.so \
+ /usr/local/lib/libjemalloc.so.2 \
+ /usr/lib/x86_64-linux-gnu/libjemalloc.so \
+ /usr/lib/x86_64-linux-gnu/libjemalloc.so.2 \
+ /usr/lib/aarch64-linux-gnu/libjemalloc.so \
+ /usr/lib/aarch64-linux-gnu/libjemalloc.so.2; do
+ if [[ -f "$p" ]]; then
+ system_lib="$p"
+ break
+ fi
+ done
+ fi
+
+ # If found, set LD_PRELOAD and return immediately
+ if [[ -n "$system_lib" ]]; then
+ if [[ ":${LD_PRELOAD:-}:" != *"libjemalloc"* ]]; then
+ export LD_PRELOAD="${system_lib}${LD_PRELOAD:+:$LD_PRELOAD}"
+ fi
+ return 0
+ fi
+
+ # Detect system architecture
+ arch=$(uname -m)
+
+ # System jemalloc not found, try to download the correct library for the
architecture
+ if [[ $arch == "aarch64" || $arch == "arm64" ]]; then
+ lib_file="$top/bin/libjemalloc_aarch64.so"
+
download_url="${GITHUB}/apache/hugegraph-doc/raw/binary-1.5/dist/server/libjemalloc_aarch64.so"
+
expected_sha256="6b7e6099b6da798829c6ce6fcb55a787508841edd52446332a73300889dcd1dc"
+ elif [[ $arch == "x86_64" ]]; then
+ lib_file="$top/bin/libjemalloc.so"
+
download_url="${GITHUB}/apache/hugegraph-doc/raw/binary-1.5/dist/server/libjemalloc.so"
+
expected_sha256="53b25e8626e1605cbd8b60befb3431cabc1b8851a54285e0dda412796feab67d"
+ else
+ echo "Unsupported architecture: $arch"
+ return 1
+ fi
+
+ # Download and verify jemalloc library (fallback when system lib not found)
+ if download_and_verify "$download_url" "$lib_file" "$expected_sha256"; then
+ if [[ ":${LD_PRELOAD:-}:" != *":${lib_file}:"* ]]; then
+ export LD_PRELOAD="${lib_file}${LD_PRELOAD:+:$LD_PRELOAD}"
+ fi
+ else
+ echo "Failed to verify or download jemalloc for $arch, skipping"
+ return 1
+ fi
+}
+
+function preload_toplingdb() {
+ local lib_dir="$1"
+ local dest_dir="$2"
+ local os_name machine_arch
+
+ # NOTE: The current ToplingDB rocksdbjni snapshot bundles Linux x86_64
native libraries.
+ # Linux arm64/aarch64 support requires a matching native rocksdbjni
artifact.
+ os_name="$(uname -s)"
+ if [ "$os_name" != "Linux" ]; then
+ echo "[common-topling] Skip ToplingDB native preload on non-Linux
platform: $os_name" >&2
+ return 0
+ fi
+ machine_arch="$(uname -m)"
+ case "$machine_arch" in
+ x86_64 | amd64)
+ ;;
+ *)
+ echo "[common-topling] Skip ToplingDB native preload on
unsupported platform: $os_name/$machine_arch" >&2
+ return 0
+ ;;
+ esac
+
+ local top="$(cd "$lib_dir"/../ && pwd)"
+
+ local jar_file
+ jar_file=$(ls -1 "$lib_dir"/rocksdbjni*.jar 2>/dev/null | sort -V | tail
-n1 || true)
Review Comment:
**High: Topling distributions cannot find their JNI JAR**
`hugegraph-server/hugegraph-dist/src/assembly/static/bin/common-topling.sh:274`
**Evidence**
- The `toplingdb` profile sets the artifact ID to `toplingdb-jni` at
`pom.xml:423`, producing `toplingdb-jni-<version>.jar`, but the preloader
searches only `rocksdbjni*.jar`.
**Impact**
- A Linux distribution built with `-Ptoplingdb` exits during initialization
or startup before launching Java.
**Requested fix**
- Search for `toplingdb-jni*.jar` when Topling is selected, or pass the
resolved engine JAR explicitly, and add a packaged startup smoke test.
##########
hugegraph-store/hg-store-rocksdb/src/main/java/org/apache/hugegraph/rocksdb/access/RocksDBSession.java:
##########
@@ -450,8 +454,11 @@ private void openRocksDB(String dbDataPath, long version) {
new
ColumnFamilyDescriptor(RocksDB.DEFAULT_COLUMN_FAMILY, cfOptions));
}
List<ColumnFamilyHandle> columnFamilyHandleList = new
ArrayList<>();
- this.rocksDB = RocksDB.open(dbOptions, dbPath,
columnFamilyDescriptorList,
- columnFamilyHandleList);
+ this.rocksDB =
+ RocksDBProviderLoader.openRocksDB(dbOptions, dbPath,
columnFamilyDescriptorList,
+ columnFamilyHandleList,
+
hugeConfig.get(RocksDBOptions.OPTION_PATH),
+
hugeConfig.get(RocksDBOptions.OPEN_HTTP));
Review Comment:
**High: HTTP startup is requested for every Store database**
`hugegraph-store/hg-store-rocksdb/src/main/java/org/apache/hugegraph/rocksdb/access/RocksDBSession.java:461`
**Evidence**
- Every session receives the global `OPEN_HTTP` value. Store creates
separate sessions for metadata and partition databases, while each Topling
repository invokes `startHttpServer()` using the same configured port.
**Impact**
- With `rocksdb.open_http=true`, opening the second database attempts to
bind the same port and prevents normal Store startup or partition access.
**Requested fix**
- Manage the HTTP server once per Store process, or pass `true` only to one
designated session.
##########
hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/ToplingRocksDBProvider.java:
##########
@@ -0,0 +1,584 @@
+/*
+ * 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.rocksdb.provider;
+
+import org.rocksdb.*;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.apache.commons.lang3.StringUtils;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Locale;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+import net.minidev.json.JSONObject;
+
+import org.yaml.snakeyaml.Yaml;
+import org.yaml.snakeyaml.constructor.SafeConstructor;
+import org.yaml.snakeyaml.LoaderOptions;
+
+/**
+ * ToplingRocksDBProvider provides ToplingDB-specific RocksDB functionality.
+ * This provider supports advanced ToplingDB features including:
+ * - YAML-based configuration via optionPath
+ * - HTTP server for monitoring and management
+ * - SidePluginRepo integration for enhanced performance
+ */
+public class ToplingRocksDBProvider extends AbstractRocksDBProvider {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ToplingRocksDBProvider.class);
+
+ private static final String PROVIDER_NAME = "topling";
+ private static final String SIDE_PLUGIN_REPO_CLASS =
"org.rocksdb.SidePluginRepo";
+
+ // Validation constants migrated from RocksDBOptions
+ private static final Pattern SAFE_PATH_PATTERN =
+ Pattern.compile("^[a-zA-Z0-9/_.-]+\\.yaml$");
Review Comment:
**Medium: Valid `.yml` configuration files disable Topling silently**
`hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/ToplingRocksDBProvider.java:58`
**Evidence**
- The path regex accepts only `.yaml`, although the later extension check
explicitly permits both `.yaml` and `.yml`. The validation exception is caught
and converted into standard RocksDB fallback.
**Impact**
- Selecting Topling with a valid `conf/*.yml` file silently ignores its
configuration and features.
**Requested fix**
- Accept both extensions in the initial regex, for example `\\.ya?ml$`, and
add a `.yml` regression test.
##########
hugegraph-rocksdb-provider/src/test/java/org/apache/hugegraph/rocksdb/provider/ToplingRocksDBProviderTest.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.rocksdb.provider;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class ToplingRocksDBProviderTest {
+
+ private ToplingRocksDBProvider provider;
+
+ @Before
+ public void setUp() {
+ provider = new ToplingRocksDBProvider();
+ }
+
+ /**
+ * Test: getProviderName() returns "topling" - the identifier used for
config matching.
+ * Users set rocksdb.provider=topling to activate this provider.
+ */
+ @Test
+ public void testProviderName() {
+ assertEquals("topling", provider.getProviderName());
+ }
+
+ /**
+ * Test: isAvailable() returns false when org.rocksdb.SidePluginRepo is
not on the classpath.
+ * In a standard test environment (using vanilla rocksdbjni), ToplingDB
features
+ * are not available. This is the expected state for most developers.
+ */
+ @Test
+ public void testIsNotAvailableWithoutSidePluginRepo() {
+ assertFalse(provider.isAvailable());
Review Comment:
**Medium: The Topling build profile contradicts its tests**
`hugegraph-rocksdb-provider/src/test/java/org/apache/hugegraph/rocksdb/provider/ToplingRocksDBProviderTest.java:53`
**Evidence**
- `-Ptoplingdb` supplies `SidePluginRepo`, but this test unconditionally
asserts that it is unavailable. Loader tests likewise require Topling selection
to fail.
**Impact**
- The documented `mvn clean package -Ptoplingdb` path cannot run its test
suite successfully and provides no positive coverage of the supported runtime.
**Requested fix**
- Isolate unavailable-provider tests with a controlled classloader and add
positive tests under the Topling profile.
##########
hugegraph-pd/hg-pd-dist/src/assembly/static/bin/start-hugegraph-pd.sh:
##########
@@ -63,9 +63,17 @@ PID_FILE="$BIN/pid"
. "$BIN"/util.sh
+PARENT_DIR="$(cd "$TOP"/../ && pwd)"
+SERVER_VERSION_DIR="${SERVER_VERSION_DIR:-$(find_hugegraph_server_dir
"$PARENT_DIR")}"
+
ensure_path_writable "$LOGS"
ensure_path_writable "$PLUGINS"
+# preload rocksdb/toplingdb
+if [ -n "$SERVER_VERSION_DIR" ] && [ -e
"$SERVER_VERSION_DIR/bin/preload-topling.sh" ]; then
Review Comment:
**Medium: Standalone PD and Store packages omit required preload support**
`hugegraph-pd/hg-pd-dist/src/assembly/static/bin/start-hugegraph-pd.sh:73`
**Evidence**
- PD sources the preloader only from a sibling server distribution; Store
does the same at `start-hugegraph-store.sh:70`. Their assembly descriptors
package no component-local Topling preload scripts.
**Impact**
- Enabling the documented Topling configuration in a standalone PD or Store
distribution leaves its native libraries and resources unprepared.
**Requested fix**
- Package component-local preload support that uses each distribution’s own
engine artifact, or explicitly reject Topling configuration for standalone
packages.
--
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]
