Repository navigation
Conversation
The Store fix for apache/hugegraph#3254 ships rocksdb.total_memory_size as 0 and sizes the budget from the memory left after the heap, direct memory and a 1.5 GiB reserve, starting from the cgroup limit in a container. The HStore page still listed the old 32000000000 value and said 0 meant the JVM max heap. Update the rocksdb.total_memory_size row with the new rule, note the minimum of 1 GB in the raft.disruptorBufferSize derivation, and add HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE to the Store env var tables in the HStore quick start and the Docker cluster guide, in English and Chinese.
…db-memory-budget-rebased
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The new rocksdb.total_memory_size text, the raft.disruptorBufferSize rounding note and the HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE rows match apache/hugegraph#3355 at 535c2d01b. One minor gap: the paired change also puts a 1 MiB floor on the derived rocksdb.write_buffer_size, and that row still gives the old formula. Evidence: read the full diff at a516891 (4 files, en and cn) and checked it against RocksdbMemoryBudget, AppConfig, RaftRocksdbOptions, RocksDBOptions, both application-pd.yml files and the Store docker-entrypoint.sh at the #3355 head. Note that #3355 is still open, so the 0 distribution value is correct only once it merges, as the PR description says.
apache/hugegraph#3355 fills an absent or 0 rocksdb.write_buffer_size with total_memory_size / 1000, but never below the 1 MiB that RocksDBOptions accepts. Any budget under about 1 GB, including the 256 MiB auto-sizing floor, therefore gets 1048576. Say so in the EN and CN rows.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The rocksdb.total_memory_size, write_buffer_size, raft.disruptorBufferSize and HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE text matches apache/hugegraph#3355 at 535c2d01b, and EN and CN agree. Two gaps remain: the Docker cluster guide says an unset variable sizes the budget from the container memory limit, but the Compose files it documents set no limit, so each Store sizes from the whole Docker host or VM; and the HStore page still says its tables were checked at master commit 2f827d6e, which predates the values this PR now lists. Evidence: read the full diff at 120c614 (4 files, EN and CN) and checked it against RocksdbMemoryBudget, AppConfig, RaftRocksdbOptions, RocksDBOptions, both application-pd.yml files, the Store docker-entrypoint.sh and start-hugegraph-store.sh at the #3355 head, the docker/*.yml Compose files (no mem_limit or deploy.resources), and application-pd.yml/AppConfig at 2f827d6e; links resolve to existing pages, git diff --check is clean and the site build is green. The earlier write_buffer_size floor comment is fixed in 120c614.
The Docker guide said an unset HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE sizes the budget from the container memory limit, but the Compose files it documents set no limit, so each Store sizes from the whole Docker host. Say so, and tell readers to set a limit or pin the variable when several Stores share a host. The HStore row now says the cgroup limit applies when one is set, otherwise the host's memory. The HStore page also said its tables were checked at a master commit from before apache/hugegraph#3355; note that the three RocksDB and disruptor rows describe that PR until its merge commit replaces it.
apache/hugegraph#3355 now budgets the JVM's real direct-memory allowance: an unset -XX:MaxDirectMemorySize counts as the heap, and bin/start-hugegraph-store.sh sets the flag to the heap it picks unless -j passes one. Drop the "at most 1 GiB" from the EN and CN rows.
apache/hugegraph#3355 now leaves -XX:MaxDirectMemorySize out when -j sets -Xmx or the cap itself, so a heap-only -j gets the JVM default. Update the EN and CN rows.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The new rocksdb.total_memory_size, write_buffer_size, raft.disruptorBufferSize and HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE text matches apache/hugegraph#3355 and the EN and CN pages agree; one minor gap remains because #3355 changed the launcher's direct-memory rule after this head. Evidence: read the full diff at b9e2b34 (4 files, EN and CN) and checked it against RocksdbMemoryBudget, AppConfig, RaftRocksdbOptions, the Store RocksDBOptions, both application-pd.yml files, the Store docker-entrypoint.sh and start-hugegraph-store.sh at the #3355 head 9e6dc506, plus the docker/*.yml Compose files (no memory limits); the site build checks on this head pass.
The launcher also leaves the direct-memory cap out when -j sets -XX:MaxHeapSize (643db96). Say so in the deployment guide, matching apache/hugegraph-doc#535.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9.5/10 (10 - 0.5 for one Minor). Summary: The rocksdb.total_memory_size, write_buffer_size, raft.disruptorBufferSize and HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE text matches apache/hugegraph#3355 at 37597c441, and the EN and CN pages agree. One minor gap: the launcher example further down the HStore page now leads to the 256 MiB floor. Evidence: read the full diff at 0bb8278 (4 files, EN and CN) and the delta since b9e2b34, and checked each statement against RocksdbMemoryBudget (1.5 GiB reserve, half of the rest, 256 MiB to 32000000000 bytes clamp, heap counted for an unset MaxDirectMemorySize, 1 MiB explicit minimum, plain-number parse, cgroup v1/v2 walk with ancestors), AppConfig, RaftRocksdbOptions (1 GiB Raft log LRUCache), both application-pd.yml files, the Store docker-entrypoint.sh (digits only, exit 2, emitted only when set) and start-hugegraph-store.sh at the #3355 head. git diff --check is clean and the site build passes at this head. This PR should merge after apache/hugegraph#3355, since the 0 distribution value and the new rules exist only there.
Earlier comments, all fixed at this head:
With apache/hugegraph#3355 a -j that sets -Xmx drops the launcher's -XX:MaxDirectMemorySize, so the JVM allows direct memory equal to the heap and the RocksDB budget counts it. The example -j "-Xmx32g -Xms32g" therefore left any host under about 66 GB on the 256 MiB floor. Add -XX:MaxDirectMemorySize=2g to the example, list the cap among the flags the script adds, and say why, in EN and CN.
A heap set through -j or JAVA_OPTS leaves -XX:MaxDirectMemorySize to the JVM, which then allows direct memory equal to the heap, and the RocksDB budget counts it. The README's -j "-Xmx16g -Xms8g" and best-practices' 32 GB heap examples would put most hosts on the 256 MiB floor. Add -XX:MaxDirectMemorySize=2g to each, matching apache/hugegraph-doc#535.
master (apache#539) adds {.td-table--wide} after the Docker env tables, right where this branch appends the HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE row. Keep the new row as the table's last row, followed by the attribute.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9.5/10. Summary: The rocksdb.total_memory_size, write_buffer_size, raft.disruptorBufferSize, HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE and launcher text matches apache/hugegraph#3355 at b9df448a, and the EN and CN pages agree. The earlier launcher-example comment is fixed in d11a103. This should still merge after apache/hugegraph#3355, since the 0 distribution value and the new rules exist only there; if #3355 changes before it merges, these rows need another check. Evidence: read the full diff at 849be33 (4 files, EN and CN; the last commit is a master merge that does not touch them) and checked each statement against RocksdbMemoryBudget (1.5 GiB reserve, half of the rest, 256 MiB to 32000000000 bytes clamp, heap counted for an unset MaxDirectMemorySize, 1 MiB explicit minimum, plain-number parse, cgroup walk including ancestors), AppConfig, RaftRocksdbOptions (one shared 1 GiB Raft log LRUCache), both application-pd.yml files, the Store docker-entrypoint.sh and start-hugegraph-store.sh, and the docker/*.yml Compose files (no memory limits). git diff --check is clean and the site build passes on this head.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The auto-sizing guidance matches the main calculation but omits the unknown-memory fallback and the limits of using a container cap alone. Two inline comments document those cases; the paired implementation PR #3355 remains open. Evidence: Compared this diff with #3355 at f4cd98c, including RocksdbMemoryBudget and the Store launcher; the exact-head Docs workflow passed.
…limit The HStore row now says what the budget falls back to when neither the JVM nor the cgroup files report a usable memory size: the JVM max heap, clamped to 256 MiB to 32 GB. The Docker guide said a Store memory limit sizes the RocksDB budget, but that is all it bounds: the launcher still picks -Xmx from the host's free memory and sets direct memory to the same size. Tell operators to size both through JAVA_OPTS for the limit, with an example, and note that the defaults already exceed a 2 GiB limit. EN and CN.
The deployment guide said an unset HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE sizes the budget from the container limit, but the launcher still picks -Xmx from the host's free memory and sets direct memory to match. Tell operators to size both through JAVA_OPTS, matching apache/hugegraph-doc#535.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The new memory guidance leaves three cases that can lead operators to retain the old 32 GB budget, misread empty-variable validation, or treat the cache budget as a hard cap. Evidence: paired Store configuration and entrypoint at apache/hugegraph#3355 head 7ded3fc7f7847e468806472eadcb9a03037e2599; details are attached inline.
- Docker guide: leaving HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE unset auto-sizes only when the loaded rocksdb.total_memory_size is missing or 0; an application-pd.yml kept from an earlier release still sets 32000000000. Link the upgrade note. - HStore env row: an empty value counts as unset; only a non-empty value must be digits. - HStore config row: the budget is a sizing target, not a hard cap. With rocksdb.write_buffer_allow_stall at its default false, native memory can exceed it under heavy writes, so an OOM is still possible. EN and CN.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9.5/10. Summary: The rocksdb.total_memory_size, rocksdb.write_buffer_size, raft.disruptorBufferSize, HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE, launcher and Docker guide text matches apache/hugegraph#3355 at 7ded3fc7, and the EN and CN pages agree. The three inline comments from the review of 781384b (the auto-sizing precondition in the Docker guide, the empty env value, the budget as a soft target) are addressed at this head. This should still merge after apache/hugegraph#3355, since the 0 distribution value, the env variable and the launcher's direct-memory cap exist only there. Evidence: read the full diff at 638a816 (4 files, EN and CN) and checked each statement against RocksdbMemoryBudget (1.5 GiB reserve, half of the rest, 256 MiB to 32000000000 bytes clamp, heap fallback when no memory size is known, 1 MiB explicit minimum, writeBufferSize 1 MiB floor, disruptorBufferSize with the GB count floored at 1), AppConfig, the Store RocksDBOptions (write_buffer_allow_stall defaults to false), RaftRocksdbOptions (1 GiB Raft log cache), both application-pd.yml files (32000000000 at the 1.5.0 and 1.7.0 tags), the Store docker-entrypoint.sh (-n gate, exit 2), start-hugegraph-store.sh and calc_xmx in util.sh, the Store Dockerfile JAVA_OPTS, and the docker/*.yml Compose files (no memory limits). dist/validate-links.sh passes and git diff --check is clean at this head. The site build for this head was still queued at review time; the previous head 781384b built green, and the change since is 10 added and 5 removed prose lines.
…db-memory-budget-rebased
The row listed 33554432 as the code default but its own text says an absent or 0 value is derived from total_memory_size, which is what AppConfig does before RocksDBOptions' default is ever used. Say "Derived, see Meaning", as the total_memory_size row already does.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 10/10. Summary: The rocksdb.total_memory_size, rocksdb.write_buffer_size, raft.disruptorBufferSize, HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE, launcher and Docker guide text matches apache/hugegraph#3355 at e8d42c11, and the EN and CN pages agree. The change since the approved head 638a816 is a master merge plus 4b6ceae, which replaces the 33554432 code default in the write_buffer_size row with "Derived, see Meaning"; that is correct, because AppConfig always fills an absent or 0 write_buffer_size with RocksdbMemoryBudget.writeBufferSize() before the RocksDBOptions.MEMTABLE_SIZE default (32 MiB) can apply. This should still merge after apache/hugegraph#3355, since the 0 distribution value, the env variable and the new rules exist only there. Evidence: read the full diff at 4b6ceae (4 files, EN and CN) and checked each statement against #3355 at e8d42c11 (its Store sources are unchanged since 7ded3fc7): RocksdbMemoryBudget (1.5 GiB reserve, half of the rest, 256 MiB to 32000000000 bytes clamp, heap counted for an unset MaxDirectMemorySize, heap fallback when no memory size is known, 1 MiB explicit minimum, plain-number parse, cgroup walk including ancestors, writeBufferSize 1 MiB floor, disruptorBufferSize GB count floored at 1), AppConfig, RocksDBOptions (write_buffer_allow_stall false, MEMTABLE_SIZE 32 MiB), RaftRocksdbOptions (1 GiB LRUCache), both application-pd.yml files, the Store docker-entrypoint.sh (-n gate, exit 2), start-hugegraph-store.sh and calc_xmx. dist/validate-links.sh passes and git diff --check is clean at this head; the site build for this head is still queued.
In PD mode the first Store uploads its application-pd.yml to PD (KV key S:FS), and every Store whose local file isn't newer loads that copy and writes it over its own. An old 32000000000 there survives a Store upgrade, so the upgrade note now says to change it in PD's copy or set the per-Store override, matching apache/hugegraph#3355. EN and CN.
apache/hugegraph#3355 now gives the Raft log DBs one shared 256 MiB write buffer charged to the fixed 1 GiB Raft log cache, so the memtables of all partitions don't add to it.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The PD-managed upgrade instructions omit the timestamp publication required for Stores to apply the new budget. Evidence: PdConfigureListener, KvClient.put and KvServiceGrpcImpl.put at the paired implementation revision de37bcb8de56fe7e92244b6d73828fd444140032 show that updating S:FS neither notifies the timestamp listener nor advances the startup freshness check.
A Store loads PD's S:FS copy only when it has no local file or the file is no newer than S:Timestamp, which the first upload never sets. Editing S:FS alone therefore changes nothing. Recommend the per-Store override set to 0, and describe the shared-copy route: change S:FS, then publish S:Timestamp so running Stores reload and restart.
|
@imbajin the |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The budget, Raft log write buffer, S:FS/S:Timestamp and override text matches apache/hugegraph#3355 at a7dc5c9a7, and the EN and CN pages agree. The alternative upgrade path omits that the PD-triggered self-restart drops the launcher flags, so a Store started with this page's production -j heap comes back with the default heap.
apache/hugegraph#3355 now ships total_memory_size: auto, keeps a missing, blank or 0 value as aliases, and lets HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE take auto. The reference rows and the Docker cluster guide say so, in English and Chinese, along with the reserve split, how a limit the Store can't see is handled, and what the startup log reports.
|
The earlier review comments below still apply at 1373759; this head does not fix them.
|
…t flags Recommend 0 or removing the key in S:FS until every Store supports auto, since an earlier release fails to start on auto. Note that the S:Timestamp self-restart drops -j and -g, including the Docker JAVA_OPTS, so only an exported JAVA_OPTIONS carries over. Both in EN and CN.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The auto value, the auto-sizing formula, the cgroup and fallback rules, the S:FS/S:Timestamp upgrade steps and the HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE rows match apache/hugegraph#3355 at 63856ac3, and the EN and CN pages agree. The self-restart note added at this head is wrong for Docker: the container exits instead of restarting without JAVA_OPTS, and a Store started without --restart stays down.
| |-----|---------|--------------|---------| | ||
| | `rocksdb.total_memory_size` | `32000000000` | `51539607552` | Memory budget shared by all RocksDB instances on this node. When absent or `0`, the node uses the JVM max heap instead. | | ||
| | `rocksdb.write_buffer_size` | `32000000` | `33554432` | Memtable size in bytes. When absent or `0`, the node uses `total_memory_size / 1000`. | | ||
| | `rocksdb.total_memory_size` | `auto` | Auto-sized, see Meaning | Memory budget, `auto` or a number of bytes, shared by all RocksDB instances on this node, split into the write cache and the block cache by `write_buffer_ratio`. Both caches are off-heap, so `-Xmx` does not limit them, and a fixed 1 GiB Raft log cache comes on top; the Raft log memtables of all partitions share a 256 MiB write buffer charged to that cache, so they don't add to it. The budget is a sizing target, not a hard cap: with `rocksdb.write_buffer_allow_stall` at its default `false`, writes are not stalled when the write cache is full, so native memory can exceed the budget under heavy writes and a container can still be OOM-killed. Setting it to `true` holds the cap more firmly at the cost of write latency. With `auto` (a missing, blank or `0` value means the same), the node uses half of the memory left after the JVM max heap, `MaxDirectMemorySize` (counted as the heap size when unset; `bin/start-hugegraph-store.sh` sets it to the heap it picks unless `-j` sets `-Xmx`, `-XX:MaxHeapSize` or the cap itself) and a 1.5 GiB reserve (the 1 GiB Raft log cache plus 512 MiB for other native memory), clamped to 256 MiB to 32 GB. In a container the starting point is the cgroup memory limit when one is set, including a limit set only on a parent cgroup the Store can see; otherwise it is the host's memory. A limit the Store can't see, such as one above a private cgroup namespace, isn't guessed: the Store uses the lowest visible limit and logs a warning, so pin the value there. The startup log shows the budget with its mode, physical limit and source, heap, direct memory, reserve and whether the cgroup view is complete. If neither the JVM nor the cgroup files report a usable memory size, the budget falls back to the JVM max heap, clamped to the same 256 MiB to 32 GB. An `application-pd.yml` kept from an earlier release still sets `32000000000`; set it to `auto` to auto-size. In PD mode PD may also keep a copy (KV key `S:FS`), uploaded by the first Store to start. A Store loads that copy over its local file only when it has no local file or the file is no newer than `S:Timestamp`; the upload sets no timestamp, and a missing one counts as 0, so changing `S:FS` alone changes nothing. To upgrade, set `HG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE=auto` or `-Drocksdb.total_memory_size=auto` on each Store and restart it; the Store keeps this value above both its local file and PD's copy. Alternatively, set the value to `0` or remove it in `S:FS`, then put the current time in milliseconds into `S:Timestamp` through PD's KV service (gRPC `KvService.put`): every running Store then reloads PD's copy, writes it over its local file and restarts itself with `bin/restart-hugegraph-store.sh` within 60 seconds, and a stopped Store loads it at its next start. Put `auto` in `S:FS` only once every Store runs a release that supports it: an earlier release can't parse `auto` and fails to start, while it reads `0` as its old heap-sized default. The same holds for a copy uploaded by a newer Store when PD had none, since the shipped file says `auto`. The self-restart passes no arguments, so `-j` and `-g` from the original start are dropped, including the `JAVA_OPTS` the Docker entrypoint passes as `-j`, and the launcher's default heap applies; only JVM settings in an exported `JAVA_OPTIONS` carry over, so prefer the per-Store override on Stores started with a custom heap. Any other value is used as given and must be plain bytes, at least `1048576` (1 MiB); a smaller value or one such as `4GB` stops startup. | |
There was a problem hiding this comment.
Important: in the Docker image this self-restart never starts a new Store, because stopping Java ends the entrypoint and dumb-init, so the container exits with the restart script inside it. Replace the JAVA_OPTS clause with that: a container with a restart policy (the Compose files use unless-stopped) comes back through the entrypoint and keeps JAVA_OPTS, while one started like the docker run -d example above, without --restart, stays down; mirror it in the CN row.
Purpose of the PR
apache/hugegraph#3355 changes the Store's shipped
rocksdb.total_memory_sizeto0. With0or no value, the Store sizes the RocksDB budget as half of what is left after the JVM max heap,MaxDirectMemorySizeand a 1.5 GiB reserve, clamped to 256 MiB to 32 GB. An unsetMaxDirectMemorySizecounts as the heap size, at most 1 GiB. Inside a container it starts from the cgroup memory limit, including a limit on a parent cgroup the Store can see. The Docker entrypoint also gainsHG_STORE_ROCKSDB_TOTAL_MEMORY_SIZE. The site still documented the old32000000000value and said that0meant the JVM max heap.Changes, in English and Chinese:
quickstart/hugegraph/hugegraph-hstore.md:rocksdb.total_memory_sizerow: the new distribution value0, the auto-sizing rule, the off-heap caches plus the separate 1 GiB Raft log cache, the 1 MiB minimum for an explicit value, and plain bytes only.application-pd.ymlkept from an earlier release still says32000000000and has to be set to0to auto-size.raft.disruptorBufferSizerow, which the code fix now guarantees.guides/hugegraph-docker-cluster.md: added the variable to the Store table, with one line pointing to the configuration reference.bash dist/validate-links.shpasses andgit diff --checkis clean. I did not build the site locally because Hugo is not installed on this machine.