feat(clp-package)!: Increase buffer_flush_threshold default from 256MiB to 4GiB.#1965
Conversation
WalkthroughThis pull request increases the LogIngestor buffer_flush_threshold from 256 MiB (268,435,456 bytes) to 4 GiB (4,294,967,296 bytes) in Python, Rust, template, and Helm configuration files, and bumps the Helm Chart version. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Line 777: The inline comment for the constant buffer_flush_threshold is
incorrect: 4096 * 1024 * 1024 equals 4 GiB, not 4.096 GiB. Update the comment
next to buffer_flush_threshold to state "4 GiB" (or adjust the numeric
expression if you intended 4.096 GiB) so the comment accurately reflects the
value used by the buffer_flush_threshold constant.
In `@components/clp-rust-utils/src/clp_config/package/config.rs`:
- Line 242: The inline comment for buffer_flush_threshold in config.rs is
incorrect: 4096 * 1024 * 1024 equals 4 GiB, not 4.096 GiB—update the comment on
the buffer_flush_threshold field (symbol: buffer_flush_threshold) to read "4
GiB" (or adjust the numeric value if you truly want ~4.096 GiB), so the comment
accurately reflects the constant's value.
In `@components/package-template/src/etc/clp-config.template.json.yaml`:
- Line 24: Update the inline comment for the buffer_flush_threshold setting: the
value 4294967296 equals 4 GiB, not 4.096 GiB, so change the comment on the
buffer_flush_threshold line (symbol: buffer_flush_threshold) to read “# 4 GiB”
or “# 4096 MiB” accordingly to correct the unit conversion.
In `@tools/deployment/package-helm/values.yaml`:
- Line 166: The inline comment for buffer_flush_threshold is inaccurate: update
the comment next to the buffer_flush_threshold: 4294967296 line to read "# 4
GiB" (the numeric constant already equals 4 GiB), i.e., change the trailing
comment text only for the buffer_flush_threshold entry to correctly state "4
GiB".
There was a problem hiding this comment.
since this PR modifies the Helm chart and the Package, i reviewed all files except components/clp-rust-utils/src/clp_config/package/config.rs
please wait for @LinZhihao-723 's review & approval before merging. the title's styling may need improvement as well
There was a problem hiding this comment.
btw, this change might be BREAKING cuz now we consume more memory? on memory-constrained systems (VPC / cgroupv2 resource-constraint containers), if users don't increase the memory limit of their resources, the log-ingestor may crash?
either way, i feel the scope of the PR should be feat than chore cuz the PR does bring benefits to users
There was a problem hiding this comment.
I'm not sure if this should be categorized as a BREAKING change:
- The buffer threshold is the total size of the metadata buffered, not the actual buffer size. I suspect this would cause visible difference to users. This PR doesn't imply the memory usage will increase from 256MB to 4GB (per buffer).
- Since we didn't change the config interface, this is not "breaking". But we probably need to draw a line on whether changing the default value should be considered as breaking.
LinZhihao-723
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The change content lgtm. Here's my suggestion for the PR title:
feat(clp-package): Increase `buffer_flush_threshold` default from 256MiB to 4GiB.
Since we use the PR title as the commit message, so I'd like to give a detailed explanation on what I've changed:
- Add
(clp-package)to indicate the scope of this PR. You can learn what scope we've used in previous PRs, but in general, you can always ask Junhao or me if you have any doubts. - "increase" to "Increase": the PR title should start with a capital letter.
- Use
to surround symbols, in this case,buffer_flush_threshold`. - Add a period at the end.
buffer_flush_threshold default from 256MiB to 4GiB.
buffer_flush_threshold default from 256MiB to 4GiB.buffer_flush_threshold default from 256MiB to 4GiB.
LinZhihao-723
left a comment
There was a problem hiding this comment.
lgtm.
Also confirmed with Kirk that this PR should be a breaking change. Added ! in the PR description to indicate that.
Description
The current log-ingestor buffering is too conservative, making the submitted compression jobs not buffer enough input logs. This PR increases the
buffer_flush_thresholdto be 4.096 GiB from 256 MiB (a 16x increase).Checklist
breaking change.
Validation performed
Summary by CodeRabbit