imbajin commented on code in PR #3260:
URL: https://github.com/apache/hugegraph/pull/3260#discussion_r4186781781
##########
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
+ // (helm/hugegraph) keeps Store rollouts on OnDelete with a manual
/v1/shardGroups barrier
+ // between Pod deletions; retire that procedure once PD reports
restoration.
Review Comment:
⚠️ Blocking: no. Summary: A restore-complete signal alone would not make
this shardGroups barrier safe to retire while stopped Stores can remain Up
until their keep-alive entry expires. Please keep the rollout checks until PD
also exposes enough liveness state to distinguish stale membership. Evidence:
Issue #3229 records shardGroups reporting full membership throughout a
150-second Store outage; the chart README requires a fresh lastHeartBeat and
full shard and leader counts before deleting the next Pod.
##########
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
+ // (helm/hugegraph) keeps Store rollouts on OnDelete with a manual
/v1/shardGroups barrier
Review Comment:
⚠️ Blocking: no. Summary: The chart's default Store strategy is
RollingUpdate; OnDelete and this manual gate apply to values-cluster.yaml,
while other production values must opt in. Please scope this breadcrumb to the
production cluster preset. Evidence: helm/hugegraph/values.yaml sets
store.updateStrategy.type=RollingUpdate; values-cluster.yaml sets OnDelete; the
README confirms that scope.
##########
hugegraph-store/hg-store-dist/src/assembly/static/conf/application-pd.yml:
##########
@@ -27,6 +27,12 @@ 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
Review Comment:
⚠️ Blocking: no. Summary: Falling back to the JVM max heap can still let
native RocksDB caches grow by another heap-sized budget, so it does not ensure
a safe container total. Please describe zero as a lower cache cap and keep
container-aware sizing or an explicit override in the fix direction. Evidence:
AppConfig.init() maps zero to Runtime.maxMemory(); issue #3254 explains that
this remains a second heap-sized native allocation in addition to heap, direct
memory, and the Raft cache.
--
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]
