bitflicker64 commented on code in PR #3260:
URL: https://github.com/apache/hugegraph/pull/3260#discussion_r4193016129
##########
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:
Fixed in aba7e0b: the TODO now scopes OnDelete and the manual barrier to the
values-cluster.yaml preset and notes the default values use RollingUpdate.
##########
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:
Fixed in aba7e0b: the TODO now says restore-complete alone does not retire
the barrier, since a stopped Store stays Up until its keep-alive expires. The
rollout checks stay until PD also exposes liveness that separates stale
membership (#3229).
##########
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:
Fixed in aba7e0b: dropped the default-0 direction. The fix direction is now
a lower default, container-aware sizing, or an HG_STORE_* mapping, and the TODO
says 0 falls back to the JVM max heap, another heap-sized native budget.
##########
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
+ # to the JVM max heap. It is the capacity of the RocksDB block and write LRU
caches
+ # (RaftRocksdbOptions), not an upfront allocation, but pinned at 32 GB those
native caches can
+ # grow past a smaller container memory limit under load and get the Store
OOM-killed. The Helm
+ # chart (helm/hugegraph) can only document the ceiling and size its cluster
preset around it.
Review Comment:
Fixed in aba7e0b: the TODO no longer says the value cannot be overridden. It
names -Drocksdb.total_memory_size=<bytes> via JAVA_OPTS (store.javaOpts) as the
current override. Not yet confirmed on a running Store; I will check the
startup log before the chart relies on it.
--
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]
