Repository navigation
Conversation
- map service identity and graphspace environment inputs - expose Helm settings with existing default behavior - validate effective identity inputs before writing config - document naming constraints and paired website guidance
- check effective graphspace names before writing config - stabilize name validation with a scoped C locale - cover new and legacy names plus locale boundaries
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3322 +/- ##
============================================
+ Coverage 52.64% 54.74% +2.10%
+ Complexity 2369 369 -2000
============================================
Files 762 503 -259
Lines 67674 48402 -19272
Branches 9189 6963 -2226
============================================
- Hits 35624 26500 -9124
+ Misses 28492 19223 -9269
+ Partials 3558 2679 -879 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The entrypoint mapping, its validation order and the chart wiring hold up, and the default render is unchanged; two small gaps remain in how the chart guards the new service identity values. Evidence: read the full diff at 83bb23d (10 files) with docker-entrypoint.sh, GraphManager (NAME_REGEX, loadServices, the graph-space load filter), HugeConfig, K8sDriver and HugeGremlinServer at that head; rendered the chart at that head with helm template (helm v4.2.4) for both comments; ran the service ID encoder from the entrypoint over values with spaces, backslashes, commas and ${...} and read them back through commons-configuration2 2.8.0 PropertiesConfiguration (the repo pins 2.10.1), all unchanged; docker / deployment-contracts and build-server / server_memory (Java 17), which run docker-entrypoint-test.sh, and helm / lint-and-render pass on this head; Analyze (java) and the three pd_store jobs were still pending. The entrypoint test was not rerun locally (it needs GNU sed), and nothing was run against a PD cluster.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The deployment change has a test coverage gap around the service ID value loaded by the Server. Evidence: The new entrypoint checks inspect the serialized properties file while the test stubs the Server startup path.
- reserve legacy identity environment variables - escape dollars before Kubernetes environment expansion - verify current HugeConfig readback in Docker CI
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The entrypoint mapping, the validation and the chart wiring read correctly, and the earlier review points (legacy names in extraEnv, $(VAR) expansion, HugeConfig readback) are addressed at this head. One problem remains on the chart side: with a non-default server.graphSpace the chart's own helm test hook can no longer pass, because it queries DEFAULT-hugegraph on every Pod and a Server assigned to another graph space does not load graphs from DEFAULT. Evidence: static read of the full diff 6aa362a..91b00d2 plus GraphManager.java (filterLoadGraphByServiceGraphSpace, loadGraphsFromMeta, graph()), HugeConfig.java, K8sDriver.java and helm/hugegraph/templates/tests/test-connection.yaml at this head; all CI checks on this head pass, including docker / deployment-contracts and helm / lint-and-render. Nothing was run locally and the finding was not reproduced on a cluster.
- merge the current Apache task and auth baseline - retain identity and auth cases in the combined fixture - compile both native readers with pinned dependencies
- expose an explicit graph alias with existing defaults - retain authenticated per-Pod checks and retry limits - validate alias input and cover missing graph failures - explain target graph creation and binding requirements
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The entrypoint mapping, validation order, chart wiring and the new server.testGraphAlias hook read correctly, and the earlier review points are addressed at this head. One gap remains: the accepted service ID character set is wider than what the Server's service registry handles, so IDs containing / or - are accepted and then mis-keyed. Evidence: read the full diff 87239c5..dd10fda (13 files) with docker-entrypoint.sh, GraphManager (loadServices, clearGraphSpace, serviceName, NAME_REGEX), ServiceMetaManager, Service, HugeConfig and K8sDriver at that head; helm unittest on the chart at that head passed (27 suites, 209 tests); helm template accepted server.serviceId=a/b and analytics-prod. The entrypoint test was not run locally (it needs GNU sed), and the docker and helm CI jobs were still queued when this was written.
| "serviceId": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "pattern": "^[ -~]+$", |
There was a problem hiding this comment.
Important: the server.serviceId pattern accepts every printable ASCII character, but the Server's own service bookkeeping uses / and - as separators, so two ordinary-looking IDs are accepted here and then mishandled after startup.
Evidence (static read at dd10fda, not run on a cluster; helm template ci helm/hugegraph --set-string server.serviceId=a/b and ...=analytics-prod both render at this head):
/:GraphManager.loadServicesstores the Server's own record throughServiceMetaManager.serviceConfKey, which joins.../GRAPHSPACE/<space>/SERVICE_CONF/<name>with/.serviceConfigs()reads the records back withkey.split("/")and keeps only the last part. A Server started withteam/analyticsinteam_ais written under.../SERVICE_CONF/team/analyticsand is loaded by every later start, and by every other Server, as the serviceanalytics. Ifteam_aalso has a realanalyticsservice, both land on the same map key and one record replaces the other in the cache.-:GraphManager.clearGraphSpacewalks the cached<space>-<service>keys withkey.split("-")and callsdropService(parts[0], parts[1]). Foranalytics-prodthat isdropService("team_a", "analytics"): the record ofanalytics-prodis left in PD when the space is cleared or dropped, and a different service namedanalyticsis the one looked up.
The entrypoint applies the same printable-ASCII check, and the new tests use hyphenated IDs (service-a, legacy-service), so this is the common case, not a corner one. The README text added here says printable ASCII round-trips unchanged, which holds for the properties file but not for the registry.
Requested change: reject / in server.serviceId (schema pattern and the entrypoint check, with a must_fail case), and for - either reject it the same way or make clearGraphSpace split with a limit of 2 (graph-space names cannot contain -). If the wider character set is intended, please state these two limits in the "Server service identity" sections of both READMEs.
Purpose of the PR
Container and Helm deployments could not pass a Server service ID or graph space into
rest-server.properties, even when callers supplied the legacy environment variables. Map the supported settings before startup and expose them in the chart. The Helm test also always queriedDEFAULT-hugegraph; let operators select an existing graph binding while retaining the default.Main Changes
HG_SERVER_SERVICE_ID/HG_SERVER_GRAPHSPACEand migrate their legacy aliases; an effective nonempty new value takes precedence.server.serviceIdandserver.graphSpacechart values, preserving existing DEFAULT behavior and avoiding duplicate identity variables.server.testGraphAliasfor an existing Gremlin graph binding. Keep the default and saved-values fallback, authentication and complete per-Pod checks; do not infer or create a graph from the assigned space.Verifying these changes
Shell and process checks, real HugeConfig readback controls, Helm tests, packaging, lint/render and schema CI checks passed. Formatting, clean compilation and independent final reviews passed. A real standalone Server with StandardAuthenticator and RocksDB confirmed that selecting an existing alias passes while missing aliases fail, with unchanged graph data. This verifies graph selection; stock PD/HStore startup, non-default discovery, authorization, isolation and Kubernetes deployment still require their own acceptance checks.
Documentation Status
Doc - Done: Docker and Helm guides in this PR. Paired website documentation: ASF #517. Coordinate both merges.Implementation illustration
Helm and container identity inputs are validated and mapped into Server properties before startup, with legacy-alias support and nonempty HG_SERVER_* values taking precedence. Helm DEFAULT values preserve the image configuration; non-default graph spaces must already exist. Configuration wiring only; live discovery, authentication and isolation still require topology validation.