bitflicker64 commented on code in PR #3187:
URL: https://github.com/apache/hugegraph/pull/3187#discussion_r3941731880
##########
docker/test-compose.sh:
##########
@@ -263,6 +291,23 @@ run_render() {
assert_ha "${RENDER_DIR}/ha.json"
assert_dev_override "${RENDER_DIR}/dev.json" \
"${RENDER_DIR}/override.json"
+
+ # An absent host value renders the entrypoint default in every topology,
+ # and a host value overrides it, so the container budget can be aligned
+ # with the healthcheck start period without editing the Compose files.
+ assert_startup_timeout "${RENDER_DIR}/standalone.json" 120 server
+ assert_startup_timeout "${RENDER_DIR}/hstore.json" 120 server
+ assert_startup_timeout "${RENDER_DIR}/ha.json" 120 server0 server1 server2
+ render_with_timeout "${RENDER_DIR}/standalone-timeout.json" 450 \
+ -f "${DOCKER_DIR}/docker-compose.yml"
+ render_with_timeout "${RENDER_DIR}/hstore-timeout.json" 450 \
+ -f "${DOCKER_DIR}/docker-compose-hstore.yml"
+ render_with_timeout "${RENDER_DIR}/ha-timeout.json" 450 \
+ -f
"${DOCKER_DIR}/docker-compose-3pd-3store-3server.yml"
+ assert_startup_timeout "${RENDER_DIR}/standalone-timeout.json" 450 server
+ assert_startup_timeout "${RENDER_DIR}/hstore-timeout.json" 450 server
+ assert_startup_timeout "${RENDER_DIR}/ha-timeout.json" 450 \
+ server0 server1 server2
Review Comment:
⚠️ These assertions cannot see the `-` vs `:-` choice the rest of the change
rests on.
Both cases they cover are satisfied by either spelling: unset renders `120`
and `450` renders `450` under `${VAR-120}` and `${VAR:-120}` alike. The
difference only shows for an explicitly empty host value, the case
`docker-entrypoint.sh` was changed to reject and that
`docker-compose.yml:37-39` and README section 7 ("an empty value included,
stops the container at startup") promise reaches the container.
Verified on this head: rewriting all three Compose files to
`${HG_SERVER_STARTUP_TIMEOUT_S:-120}` still gives `Compose render contracts
passed`, while the empty host value silently renders `"120"` instead of `""`.
With the block below the suite passes as it stands and fails that variant with
`expected server HG_SERVER_STARTUP_TIMEOUT_S= in .../standalone-empty.json`.
```suggestion
assert_startup_timeout "${RENDER_DIR}/ha-timeout.json" 450 \
server0 server1 server2
render_with_timeout "${RENDER_DIR}/standalone-empty.json" "" \
-f "${DOCKER_DIR}/docker-compose.yml"
render_with_timeout "${RENDER_DIR}/hstore-empty.json" "" \
-f "${DOCKER_DIR}/docker-compose-hstore.yml"
render_with_timeout "${RENDER_DIR}/ha-empty.json" "" \
-f
"${DOCKER_DIR}/docker-compose-3pd-3store-3server.yml"
assert_startup_timeout "${RENDER_DIR}/standalone-empty.json" "" server
assert_startup_timeout "${RENDER_DIR}/hstore-empty.json" "" server
assert_startup_timeout "${RENDER_DIR}/ha-empty.json" "" \
server0 server1 server2
```
##########
docker/docker-compose-hstore.yml:
##########
@@ -92,6 +92,7 @@ services:
HG_SERVER_REST_URL: http://server:8080
HG_SERVER_MIN_FREE_MEMORY: "0"
HG_SERVER_INIT_STORE_ENABLED: "false"
+ HG_SERVER_STARTUP_TIMEOUT_S: ${HG_SERVER_STARTUP_TIMEOUT_S-120}
Review Comment:
🧹 The reason for the missing colon lives in only one of the three files.
`docker-compose.yml:37-39` explains it. Here, and at
`docker-compose-3pd-3store-3server.yml:71`, a bare
`${HG_SERVER_STARTUP_TIMEOUT_S-120}` sits directly above
`HG_SERVER_AUTH_TOKEN_SECRET: ${HUGEGRAPH_AUTH_TOKEN_SECRET:-}`, so to a later
editor it reads as a dropped colon next to a neighbour that has one. Since the
render suite cannot currently catch that edit, the comment is the cheap half of
the guard.
```suggestion
# Unset-only default: ":-" would turn an empty host value into 120
# silently, which the entrypoint rejects on purpose.
HG_SERVER_STARTUP_TIMEOUT_S: ${HG_SERVER_STARTUP_TIMEOUT_S-120}
```
The HA anchor at line 71 wants the same two lines at its two-space indent.
##########
docker/test-compose.sh:
##########
@@ -43,6 +44,33 @@ render() {
compose_auth "$@" config --format json > "${output}"
}
+# Same render with an explicit host value, to prove the variable reaches the
+# Server environment rather than only defaulting there.
+render_with_timeout() {
+ local output="$1" timeout="$2"
+ shift 2
+ env HG_SERVER_STARTUP_TIMEOUT_S="${timeout}" \
Review Comment:
🧹 `render_with_timeout` re-lists `compose_auth`'s four environment variables
instead of reusing it, so the two drift apart on the next variable added.
Lines 52-56 are a verbatim copy of lines 33-37 with `env -u` swapped for a
value. Anything added to `compose_auth` has to be added here too or `render`
and `render_with_timeout` stop rendering the same stack. Not hypothetical: your
own note on this PR says #3189 makes the Compose files require
`HG_PD_AUTH_SECRET_KEY`, at which point `render_with_timeout` fails on the two
HStore topologies unless it is edited as well.
One definition removes the second place to remember. Verified with exactly
this text in place: `bash docker/test-compose.sh render` still prints `Compose
render contracts passed`.
```bash
STARTUP_TIMEOUT_ENV=(-u HG_SERVER_STARTUP_TIMEOUT_S)
compose_auth() {
env "${STARTUP_TIMEOUT_ENV[@]}" \
HUGEGRAPH_VERSION="${VERSION}" \
HUBBLE_IMAGE="${RENDER_HUBBLE_IMAGE}" \
HUGEGRAPH_ADMIN_PASSWORD="${PASSWORD}" \
HUGEGRAPH_AUTH_TOKEN_SECRET="${SECRET}" \
docker compose "$@"
}
render_with_timeout() {
local output="$1" timeout="$2"
shift 2
(
STARTUP_TIMEOUT_ENV=(HG_SERVER_STARTUP_TIMEOUT_S="${timeout}")
compose_auth "$@" config --format json
) > "${output}"
}
```
##########
hugegraph-server/hugegraph-dist/docker/README.md:
##########
@@ -144,3 +144,35 @@ native `HEALTHCHECK` instructions. `docker ps` shows real
health status:
| `hugegraph/hugegraph-store` | `GET /v1/health` on port 8520 |
The entrypoints supervise the Java process directly — when Java exits, the
container exits. If started with a restart policy (the provided compose files
use `restart: unless-stopped`), Docker will bring it back automatically. The
old cron-based monitor (`-m true`) is for VM/bare-metal deployments only and is
not used in Docker images.
+
+## 7. Server Startup Timeout
+
+The entrypoint gives the Server a fixed budget to answer on its REST port and
+ends the container when the budget runs out. It is 120 seconds by default. Set
+`HG_SERVER_STARTUP_TIMEOUT_S` to a whole number of seconds between 1 and 86400
+to change it:
+
+```bash
+docker run -itd --name=graph -p 8080:8080 -e HG_SERVER_STARTUP_TIMEOUT_S=450
hugegraph/hugegraph:1.7.0
+```
+
+The Compose topologies under `docker/` pass the variable through to every
+Server with an unset-only default, so `HG_SERVER_STARTUP_TIMEOUT_S=450 docker
+compose up -d` raises the budget without editing a Compose file, while leaving
+it unset renders the same 120.
Review Comment:
🧹 The variable is a Compose knob now, but `docker/README.md`, the file
Compose users actually read, never mentions it.
That document walks a user through `.env` (line 26),
`HUGEGRAPH_ADMIN_PASSWORD` (42), `HUGEGRAPH_AUTH_TOKEN_SECRET` (42),
`HUBBLE_PUBLISH_HOST` (56), `HUGEGRAPH_VERSION` (231) and `HUBBLE_IMAGE` (238),
and carries the `up -d --wait` commands this timeout governs. It is not in this
PR's diff, so a reader who never opens the Server docker README has no way to
learn the knob exists.
Please add a line to `docker/README.md` beside the other host variables,
pointing here for the accepted range and the health-check note.
##########
docker/docker-compose.yml:
##########
@@ -34,6 +34,10 @@ services:
PASSWORD: ${HUGEGRAPH_ADMIN_PASSWORD:-}
HG_SERVER_AUTH_TOKEN_SECRET: ${HUGEGRAPH_AUTH_TOKEN_SECRET:-}
HG_SERVER_MIN_FREE_MEMORY: "0"
+ # Unset-only default: a host value overrides it, an absent one
+ # renders the entrypoint default. ":-" would turn an empty host
+ # value into 120 silently, which the entrypoint rejects on purpose.
+ HG_SERVER_STARTUP_TIMEOUT_S: ${HG_SERVER_STARTUP_TIMEOUT_S-120}
Review Comment:
🧹 Under Compose the alignment this knob buys only works downward, which is
worth one word in the comment at `docker/test-compose.sh:296-297`.
Lowering the container budget to meet the health budget needs no file edit.
Raising it past the health budget cannot be done from the host at all:
`interval: 10s`, `retries: 30`, `start_period: 60s` (lines 45-50 here,
`docker-compose-hstore.yml:100-105`, and the `x-server-common` anchor at
`docker-compose-3pd-3store-3server.yml:86-91`) are literals, so the container
is `unhealthy` around 360 seconds. `docker/README.md` starts every topology
with `up -d --wait` (lines 76, 122, 175) and `hubble` gates on
`service_healthy`, so a 450 set through Compose still gives up near 360
seconds. Section 7 says as much for the reader; the test comment reads as
alignment in both directions.
Interpolating the Server health budget, say `start_period:
${HG_SERVER_HEALTH_START_PERIOD:-60s}`, would make it symmetric, but those
lines are untouched here, so a follow-up issue is the right home. Just
narrowing the comment is enough for this 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: [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]
