bitflicker64 commented on code in PR #3260:
URL: https://github.com/apache/hugegraph/pull/3260#discussion_r4185467385
##########
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:
Minor: This TODO says the value needs to become overridable and that the
chart can only document the ceiling. It can already be overridden with a JVM
system property, and the chart can set one.
- `AppConfig.RocksdbConfig` binds `rocksdb` as a `Map<String, String>` with
`@ConfigurationProperties(prefix = "")` (AppConfig.java around line 309).
Spring Boot merges map entries across property sources and lets the
higher-precedence source win for each key, and Java system properties rank
above `application-pd.yml`. With the Store's Spring Boot 2.5.14 `Binder`, a
system-property source holding `rocksdb.total_memory_size=8000000000`, listed
ahead of a yml source holding 32000000000, bound to
`{total_memory_size=8000000000, write_buffer_size=32000000}`.
- In the image, the system property reaches that binding.
`docker-entrypoint.sh` runs `start-hugegraph-store.sh -j "${JAVA_OPTS}"`.
Because `JAVA_OPTIONS` is unset in the image, the script appends `USER_OPTION`
to `JAVA_OPTIONS`, which goes on the `java` command line before `-jar`. The
chart renders `store.javaOpts` as `JAVA_OPTS` (`hugegraph.javaOptsEnv`). So
`store.javaOpts: "-Drocksdb.total_memory_size=4000000000"` lowers the cache
capacity today.
Issue #3254 lists this route as an untested workaround, and the chart README
(Limitations) and the `values-cluster.yaml` comment both say the chart cannot
lower the value. If this breadcrumb repeats that claim, the next reader at this
site will think no override exists. The real fix is still the default of 0, or
an `HG_STORE_*` mapping. Suggested wording:
```suggestion
# TODO: default this to 0 so AppConfig falls back to the JVM max heap, or
map it from an
# HG_STORE_* variable in the Docker entrypoint. 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. Until then the only override is the JVM system property
# -Drocksdb.total_memory_size=<bytes> (JAVA_OPTS, the Helm chart's
store.javaOpts).
```
Before relying on it in the chart, please confirm on a running Store that
the startup log prints the lowered `total_memory_size`, because my check was
the binder alone and not a full Store boot.
--
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]
