DNM: Add page compression with zstd#373
Conversation
openshift-ci
Bot
added
the
do-not-merge/hold
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR updates Go module dependencies in multiple Changes
Sequence Diagram(s)(omitted — changes do not introduce multi-component sequential control flow requiring a diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tjungblu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/storage/backend/backend.go (1)
504-506: Compression enabled on temporary database during defragmentation.The implementation correctly applies compression only to the temporary database created during defragmentation:
options.Mlock = falseensures the temp database isn't locked in memory (line 504)options.Compression = trueenables zstd compression (lines 505-506)The existing test at
server/storage/backend/backend_test.go:160-196validates that defragmentation preserves hash integrity and reduces file size, which will also verify compression works correctly.For future production readiness, consider making compression configurable via
BackendConfigrather than hardcoding it.🔄 Optional refactor: Make compression configurable
For future production readiness, consider adding a configuration option:
// In BackendConfig, add: // Compression enables zstd compression for the backend database. // When enabled, pages are compressed using zstd algorithm. Compression bool // In defrag(), use the config: options.Compression = bcfg.CompressionThis would allow compression to be enabled/disabled based on user requirements and provide backward compatibility options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/storage/backend/backend.go` around lines 504 - 506, The temp database used during defragmentation currently hardcodes options.Compression = true; update BackendConfig to include a Compression bool (e.g., add Compression field to BackendConfig) and modify defrag() to set options.Compression = bcfg.Compression (or the appropriate BackendConfig instance) instead of hardcoding true so compression can be toggled per backend; ensure any tests that expect compression behavior are updated or left unchanged by default config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/storage/backend/backend.go`:
- Around line 504-506: The temp database used during defragmentation currently
hardcodes options.Compression = true; update BackendConfig to include a
Compression bool (e.g., add Compression field to BackendConfig) and modify
defrag() to set options.Compression = bcfg.Compression (or the appropriate
BackendConfig instance) instead of hardcoding true so compression can be toggled
per backend; ensure any tests that expect compression behavior are updated or
left unchanged by default config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f00ba72-e319-4fc7-a8f6-837d8498f7d1
⛔ Files ignored due to path filters (4)
etcdutl/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
etcdutl/go.modgo.modserver/go.modserver/storage/backend/backend.gotests/go.mod
|
/payload 5.0 nightly blocking |
|
@tjungblu: trigger 13 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/059f2b10-4245-11f1-8a9c-3abd1d77f7fd-0 |
This enables the bbolt compression raised upstream in etcd-io/bbolt#1159 Do not merge, this is only for testing purposes. /hold Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
|
/payload 5.0 nightly blocking |
|
@tjungblu: trigger 13 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/918c1750-4245-11f1-9783-322de2dd3922-0 |
|
/payload 5.0 nightly blocking |
|
@tjungblu: trigger 13 job(s) of type blocking for the nightly release of OCP 5.0
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f06a1c40-42d1-11f1-801e-aff86dbe7c00-0 |
1 participant
This enables the bbolt compression raised upstream in etcd-io/bbolt#1159
Do not merge, this is only for testing purposes.
/hold
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
Summary by CodeRabbit
New Features
Chores