bitflicker64 commented on code in PR #3260:
URL: https://github.com/apache/hugegraph/pull/3260#discussion_r4179127864
##########
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:
Fixed in 924ec6bee. Right, `PASSWORD` already maps to `auth.admin_pa` here,
so the TODO now names only `server.urls_to_pd` and `server.deploy_in_k8s` as
missing mappings. It also says a read-only root filesystem needs the conf files
on a writable path, because `set_prop` in this script edits them in place, so
the env mappings alone would not let the chart drop its
`readOnlyRootFilesystem` refusal.
##########
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:
Resolved by bd4ba9a1c, a merge of `master`: the chart landed in #3218, so
`helm/hugegraph` now exists in this branch's tree and every reference resolves
as a repository path. I kept the paths rather than linking #3218, since they
stay correct after this PR merges and the PR link would point at a closed
review. The PR description now calls this a follow-up to the merged chart.
##########
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:
Fixed in 924ec6bee. The TODO now says `total_memory_size` is the capacity of
the RocksDB block and write LRU caches (`RaftRocksdbOptions`), not an upfront
allocation, and that those native caches can grow past a smaller container
limit under load and get the Store OOM-killed. 245134d1a links the tracking
issue, #3254.
##########
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:
Fixed in 924ec6bee. The TODO now scopes the change to the probe view and
therefore `/v1/ready`, states that `/v1/health` does not read that view, and
points to the separate TODO at `StoreAPI.checkHealthy` for making `/v1/health`
report the state.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManager.java:
##########
@@ -159,6 +159,10 @@ private void invalidateUserCache() {
}
private void invalidatePasswordCache(Id id) {
+ // TODO: invalidate the password and token caches on every Server
replica, not only on the
Review Comment:
Fixed in 924ec6bee. The TODO moved to
`StandardAuthManagerV2.invalidatePasswordCache`, which a Server on the PD
catalog runs, and the one in `StandardAuthManager` is gone. While moving it I
checked why replicas miss the change: `AuthMetaManager.updateUser` publishes no
`AuthEvent`, and nothing calls `MetaManager.listenAuthEvent`, so the TODO now
names that fix (publish a USER update event and listen for it here).
--
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]
