bitflicker64 commented on code in PR #3401:
URL: https://github.com/apache/hugegraph/pull/3401#discussion_r4368818890
##########
hugegraph-server/Dockerfile:
##########
@@ -72,8 +113,22 @@ COPY
hugegraph-server/hugegraph-dist/docker/scripts/detect-storage.groovy ./scri
COPY hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh .
RUN chmod 755 ./docker-entrypoint.sh
+# Download only during an explicit Topling build, verify and prepare with the
+# distribution's normal capability probe. No network access is needed at
startup.
+ARG TOPLING_JNI_URL
+ARG TOPLING_JNI_SHA256
+COPY docker/prepare-topling-image.sh docker/topling-runtime.env
docker/toplingdb.yaml /tmp/
+RUN --mount=type=secret,id=proxy_ca,required=false \
+ --mount=type=bind,from=topling-jni,target=/run/topling-jni \
+ if [ "$RUNTIME_VARIANT" = topling ]; then \
+ if [ -f /run/secrets/proxy_ca ]; then export
CURL_CA_BUNDLE=/run/secrets/proxy_ca; fi; \
+ bash /tmp/prepare-topling-image.sh; \
+ fi \
+ && rm /tmp/prepare-topling-image.sh /tmp/topling-runtime.env
/tmp/toplingdb.yaml
+
EXPOSE 8080
-VOLUME /hugegraph-server
+# The default data and WAL share this directory. Keep lib/bin/conf image-owned.
+VOLUME /hugegraph-server/rocksdb-data
Review Comment:
Important: Graphs created through the REST API on the standalone image are
lost on the first image change after this PR. With
`graphs.enable_dynamic_create_drop=true` (the default),
`GraphManager.createGraph()` in non-PD mode calls `StandardHugeGraph.create()`,
which writes `./conf/graphs/<name>.properties` through
`ConfigUtil.writeToFile()`. That file is outside
`/hugegraph-server/rocksdb-data`, and the supplied `docker/docker-compose.yml`
mounts only that path. Before this change the image's `VOLUME
/hugegraph-server` kept `conf/` across a Compose recreate. Compose carries an
old anonymous volume over only when the new image still declares that path, so
after `docker compose up` with this image the definition is gone, the graph is
no longer loaded, and its RocksDB files stay orphaned in the data volume. I
reproduced the Compose side with Compose 5.1.4: replacing an image that
declares `VOLUME /app` with one that declares `VOLUME /app/data`, with a named
volume on `/app/data`, ke
eps `/app/data` but drops a file written under `/app/conf`. This hits the
documented standard-to-Topling switch and a plain standard upgrade alike, and
the new README text tells users of the supplied Compose files that their data
is already mounted. Please give API-created graph definitions a persisted home
(for example, point `graphs` at a directory under the data volume and seed the
shipped `hugegraph.properties` there when it is missing), or at least state in
the upgrade notes that `conf/graphs/*.properties` must be copied out of the old
anonymous volume before the switch.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -228,6 +234,17 @@ esac
# filesystem also needs the conf files on a writable path, since set_prop in
this script edits them
# in place; the chart refuses readOnlyRootFilesystem until then.
[[ -n "${HG_SERVER_BACKEND:-}" ]] && set_prop "backend"
"${HG_SERVER_BACKEND}" "${GRAPH_CONF}"
+# Apply the image's selected JNI provider to every embedded graph in the normal
+# container graphs directory, including mounted additional graph definitions.
+# Distributed graphs keep backend=hstore and do not select a local provider.
+if [[ -n "${TOPLINGDB_ROCKSDB_PROVIDER:-}" ]]; then
+ for graph_conf in ./conf/graphs/*.properties; do
+ [[ -f "${graph_conf}" ]] || continue
+ if [[ "$(get_prop_encoded backend "${graph_conf}")" == rocksdb ]]; then
+ set_prop "rocksdb.provider" "${ROCKSDB_PROVIDER}" "${graph_conf}"
Review Comment:
Important: In the Topling standalone image, creating a graph through the
REST API fails unless the request sets `rocksdb.provider=topling` by hand. The
image exports `TOPLINGDB_ROCKSDB_PROVIDER=topling`, so the launcher loads the
Topling JNI classes, but this loop rewrites only the `.properties` files
present at container start. A graph created later through `POST
/graphspaces/DEFAULT/graphs/{name}` takes its config only from the request
(`GraphManager.createGraph()` to `createGraphLocal()`),
`RocksDBOptions.PROVIDER` defaults to `rocksdb`, and
`RocksDBStdSessions.openRocksDB()` then calls
`RocksDBRuntime.verify("rocksdb")`, which throws because
`org.rocksdb.SidePluginRepo` comes from the same jar as `org.rocksdb.RocksDB`.
That is the same mismatch this loop exists to prevent for the startup graphs,
and the docs tell users the image sets the business provider for them. Please
make runtime-created RocksDB graphs follow the selected provider (for example,
have `createGraphLocal()` f
ill in `rocksdb.provider` from `TOPLINGDB_ROCKSDB_PROVIDER` when
`backend=rocksdb` and the key is absent), and add a test that creates a graph
with that variable set.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -228,6 +234,17 @@ esac
# filesystem also needs the conf files on a writable path, since set_prop in
this script edits them
# in place; the chart refuses readOnlyRootFilesystem until then.
[[ -n "${HG_SERVER_BACKEND:-}" ]] && set_prop "backend"
"${HG_SERVER_BACKEND}" "${GRAPH_CONF}"
+# Apply the image's selected JNI provider to every embedded graph in the normal
+# container graphs directory, including mounted additional graph definitions.
+# Distributed graphs keep backend=hstore and do not select a local provider.
+if [[ -n "${TOPLINGDB_ROCKSDB_PROVIDER:-}" ]]; then
Review Comment:
Important: The documented return to the standard image fails for any graph
config that is mounted into the container. With
`TOPLINGDB_ROCKSDB_PROVIDER=topling` this loop writes
`rocksdb.provider=topling` into every local graph file, mounted ones included,
and the standard image leaves that value alone because the variable is unset
there (the new case in `docker-entrypoint-test.sh` asserts that
`analytics.properties` still says `topling`). The standard launcher loads stock
RocksDB, so `RocksDBRuntime.verify("topling")` in
`RocksDBStdSessions.openRocksDB()` stops the Server at startup.
`docs/storage-lifecycle.md` says the unsuffixed image configures the launcher
and provider together, and `docker/README.md` describes the return as the same
stop/up sequence; both hold only while `conf/graphs` is image-owned. Please
either have the standard image reset `rocksdb.provider` to `rocksdb` on the
local graphs it rewrites, or document that a mounted graph config must be set
back to `rocksdb.
provider=rocksdb` before the return, and cover that case in the entrypoint
test.
--
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]
