imbajin commented on code in PR #3260:
URL: https://github.com/apache/hugegraph/pull/3260#discussion_r4172621244
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -146,6 +146,10 @@ if [[ -n "${PASSWORD:-}" && -z
"${HG_SERVER_AUTH_TOKEN_SECRET:-}" ]]; then
fi
# ── Map env → properties file ─────────────────────────────────────────
+# TODO: map server.urls_to_pd, server.deploy_in_k8s and auth.admin_pa from the
environment here.
Review Comment:
🧹 Blocking: no. Summary: This TODO says auth.admin_pa still needs an
environment mapping, but this script already maps PASSWORD to auth.admin_pa at
lines 175–176. The chart wrapper is also not the only read-only filesystem
blocker because this entrypoint continues editing configuration through
set_prop. Please remove auth.admin_pa from the missing list and clarify the
remaining writable-configuration requirement. Evidence: the existing PASSWORD
branch and set_prop calls in this file.
##########
hugegraph-store/hg-store-dist/src/assembly/static/conf/application-pd.yml:
##########
@@ -27,6 +27,10 @@ management:
rocksdb:
# rocksdb total memory usage, force flush to disk when reaching this value
+ # TODO: make this overridable from the environment, or default it to 0 so
AppConfig falls back
+ # to the JVM max heap: pinned at 32 GB, a Store in a container with a
smaller memory limit is
Review Comment:
🧹 Blocking: no. Summary: The configured 32 GB is an LRUCache capacity, not
memory allocated up front, so a smaller container limit does not by itself
guarantee an OOM kill. Please say cache growth can exceed the container limit
and cause OOM under load. Evidence: RaftRocksdbOptions passes total_memory_size
as the capacities of two LRUCache instances.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftStateMachine.java:
##########
@@ -167,6 +167,11 @@ public void onLeaderStart(final long term) {
@Override
public void onLeaderStop(final Status status) {
this.leaderTerm.set(-1);
+ // TODO: keep STATE_ERROR set by onError instead of overwriting it
with STATE_FOLLOWER, so
+ // the probe view (and /v1/health, see StoreAPI.checkHealthy) can
report a PD that stepped
Review Comment:
🧹 Blocking: no. Summary: Preserving STATE_ERROR here affects the probe view
used by readiness, but StoreAPI.checkHealthy still returns 200 without
consulting Raft state. Please scope this TODO to /v1/ready and point to the
separate StoreAPI health TODO. Evidence: checkHealthy returns an empty string
and its Javadoc says it does not read the Raft state.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java:
##########
@@ -158,6 +158,12 @@ public Metapb.Store register(Metapb.Store store) throws
PDException {
}
// offline or up, or in the initial activation list, go live
automatically
+ // TODO: do not mark a re-registering Store Up before it has restored
its partition engines
+ // (HgStoreEngine.restoreLocalPartitionEngine); report a restoring
state, or expose
+ // restore-complete per shard group, so a rolling restart can wait on
it. The Helm chart
Review Comment:
🧹 Blocking: no. Summary: The relative helm/hugegraph path is absent from
this PR's tree, so readers cannot follow the rollout procedure from this
revision. Please link the companion chart source or
[apache/hugegraph#3218](https://github.com/apache/hugegraph/pull/3218), and
update the other new helm/hugegraph references too. Evidence: helm/hugegraph is
added only in the separate, still-open chart 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]
