bitflicker64 commented on code in PR #3134:
URL: https://github.com/apache/hugegraph/pull/3134#discussion_r3954363272
##########
hugegraph-server/hugegraph-dist/pom.xml:
##########
@@ -112,6 +112,9 @@
<resource>
<directory>${basedir}/src/assembly/static</directory>
<filtering>true</filtering>
+ <excludes>
+ <exclude>lib/topling/**</exclude>
Review Comment:
‼️ This exclusion exists to stop Maven resource filtering from corrupting a
binary that should not be in the tree.
`src/assembly/static/lib/topling/rocksdbjni-8.10.2-20260725.141011-1.jar` is
9.5 MB (`size: 9522363` from the contents API) and it is a timestamped
SNAPSHOT: unreproducible, and it lands in the source release.
You already added the repository it comes from, three files over in
`.github/configs/settings.xml`:
```xml
<url>https://maven.pkg.github.com/hugegraph/toplingdb</url>
```
Declare it as an optional/profile-scoped dependency, or `curl` it in
`prepare-topling.sh` from that same URL. Then the JAR, this `<excludes>`, the
matching `<exclude>lib/topling/**</exclude>` in `assembly.xml`, the 202-line
`LICENSE-rocksdbjni-8.10.2-SNAPSHOT.txt`, and the `ls -1
"$lib_dir"/rocksdbjni*.jar | sort -V | tail -n1` pick in `common-topling.sh`
all go away in one move.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/verify-rocksdb-provider.sh:
##########
@@ -0,0 +1,148 @@
+#!/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
+
+if [ "$#" -lt 3 ] || [ "$#" -gt 4 ]; then
+ echo "Usage: $0 <server|pd|store> <rocksdb|topling> <data-paths>
[enforce]" >&2
+ exit 2
+fi
+
+COMPONENT="$1"
+PROVIDER="$2"
+DATA_PATHS="$3"
+ENFORCE="${4:-false}"
+MARKER_NAME=".hugegraph-rocksdb-provider"
Review Comment:
⚠️ Keep the marker - refusing to open Topling data as plain RocksDB is the
right guard. It is the 148 lines around it I would cut.
`flock` plus `/proc/self/fd` pinning plus two `stat -Lc '%d:%i'` TOCTOU
re-checks defends against someone racing the data directory. That is not the
failure being prevented; the failure is an operator flipping one line in a
config file. And it buys Linux-plus-GNU-coreutils-only for a feature that is
already Linux x86_64 only in a different way.
More to the point, `grep -rn verify-rocksdb-provider.sh` finds callers in
exactly three places: the Server, PD and Store `docker-entrypoint.sh`. A
tarball install - `bin/start-hugegraph-pd.sh`, `init-store.sh`, the test
harness - never runs it, so the guard is absent precisely where hand-edited
config files live.
`RocksDBStore.open()` already reads `RocksDBOptions.PROVIDER` and is the one
place the DB is actually opened. Write and compare the marker file under
`this.dataPath` right there, next to the existing `PROVIDER` read. A few lines,
every launch path covered, and this script plus the `<file>` entries added to
both `server-assembly.xml` descriptors go away.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/common-topling.sh:
##########
@@ -0,0 +1,288 @@
+#!/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
+ local resource_target="$dest_dir/rocksdb_resource"
+
+ if [ ! -f "$jar_file" ]; then
+ echo "Error: JAR file '$jar_file' does not exist." >&2
+ return 1
+ fi
+
+ mkdir -p "$resource_target" || {
+ echo "Error: Cannot create resource directory '$resource_target'." >&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 "$resource_target" >
/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
+ }
+
+}
+
+function ensure_libaio_symlink() {
+ local dest_dir="$1"
+ # Keep the Ubuntu 24.04 compatibility link inside the component runtime.
+ # Installation must never require sudo or modify /usr/lib.
+ 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" &&
+ [ ! -e /usr/lib/x86_64-linux-gnu/libaio.so.1 ] &&
+ [ -e /usr/lib/x86_64-linux-gnu/libaio.so.1t64 ]; then
+ mkdir -p "$dest_dir"
+ ln -sfn /usr/lib/x86_64-linux-gnu/libaio.so.1t64 \
+ "$dest_dir/libaio.so.1"
+ echo "Prepared component-local libaio.so.1 compatibility link"
+ fi
+ fi
+}
+
+function download_and_verify() {
Review Comment:
⚠️ `download_and_verify` already exists at
`hugegraph-store/hg-store-dist/src/assembly/static/bin/util.sh:310`, and the
new `<file>` entry in
`hg-store-dist/src/assembly/descriptor/server-assembly.xml` drops
`common-topling.sh` into that same `bin/`. So on Store this definition silently
overrides the one `start-hugegraph-store.sh` sourced ~30 lines earlier, and the
two disagree on whether argument 3 is an MD5 or a SHA-256.
The copy is also the weaker of the two. The existing one downloads to
`mktemp` and `mv -f`, with a comment saying why:
```bash
# mktemp, not $$: a PID is shared by concurrent background subshells.
```
This one does `rm -f "$filepath"` and then `curl -o "$filepath"` - the exact
non-atomic shape that comment was added to remove.
Source `util.sh` and teach the existing function a sha256 mode, instead of
shipping a second implementation next to it. Same for
`download_and_setup_jemalloc` below: `start-hugegraph-store.sh:42-64` already
does the arch-select, the download and the `LD_PRELOAD`, and your version
short-circuits on `LD_PRELOAD` anyway, so on Store it is 40 lines that never
run.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -857,6 +857,18 @@ private MatchedIndex collectMatchedIndex(SchemaLabel
schemaLabel,
private ConditionQuery constructSearchQuery(ConditionQuery query,
MatchedIndex index) {
ConditionQuery newQuery = query;
+ ConditionQuery filterQuery = query;
+ Query rootQuery = query.rootOriginQuery();
Review Comment:
‼️ This is a search-index filter fix - checking an `IN` condition against
the root query instead of the flattened `EQ` sub-query. It has nothing to do
with `rocksdb.provider`.
Same for the transaction-leak set that rides along with it:
`HugeFactory.closeCurrentThreadTransactions`,
`StandardHugeGraph.closeCurrentThreadTransaction`, `OltpTraverser.close()`, the
`cleanup` hook on `HugeGraphAuthProxy.ContextTask`, the two new
`HugeFactoryAuthProxy` reflection registrations, and the new `HugeFactoryTest`
/ `ContextTaskTest` / `OltpTraverserTest`. That is roughly 400 lines, applied
unconditionally to every provider, inside a PR whose description only claims an
opt-in storage engine and states "No public HugeGraph API changes".
Split them into their own PR - they will get a review that is actually about
them, and this one shrinks to the ToplingDB change. If any of it is a ToplingDB
prerequisite, name which and why in the description; right now nothing in the
PR explains it.
##########
hugegraph-pd/hg-pd-dist/src/assembly/static/conf/rocksdb_pd.yaml:
##########
@@ -0,0 +1,162 @@
+#
+# 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.
+#
+# common parameters
+http:
+ # normally parent path of db path
+ document_root: ./library/rocksdb_resource
+ listening_ports: '127.0.0.1:2012'
Review Comment:
⚠️ This file, `hugegraph-store/.../conf/rocksdb_store.yaml` and
`hugegraph-server/.../conf/toplingdb.yaml` are byte-identical apart from this
one line:
```
$ diff rocksdb_pd.yaml toplingdb.yaml
22c22
< listening_ports: '127.0.0.1:2012'
---
> listening_ports: '127.0.0.1:2011'
```
(`rocksdb_store.yaml` differs only by `2013`.) That is 486 new lines to vary
a port three ways. The description says "component-specific configurations for
Server, PD, and Store, including separate HTTP ports and JRaft column-family
settings" - the column-family settings are identical too.
Ship one `toplingdb.yaml`. `preload-topling.sh` already computes the
component and exports `TOPLINGDB_EASY_MIGRATE_CONF`; have it export the port
alongside, or template the single file through the resource filtering the
assembly already applies to `static/`. 324 lines deleted.
While you are in there: lines 24-30 are ToplingDB's upstream sample
placeholders, shipped verbatim as production config in all three copies -
```yaml
setenv:
StrSimpleEnvNameNotOverwrite: StringValue
OverwriteThisEnv:
value: force overwrite this env by overwrite true
```
--
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]
