-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: compaction support #282
Conversation
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
If we enable this, then the built-in compaction loop doesn't serve much purpose. Can we perhaps put this behind a CLI flag, and disable the built-in compaction loop when client-initiated compaction is enabled? |
In my use case the default 5m interval is fine 99% of the time. When a certain situation arises, I trigger a couple of manual compactions over a shorter time period, then fall back to the default. I'm totally open to adding a flag that disables the built-in compaction loop, but would you consider allowing both? |
upstream etcd has:
right now, kine's behavior is essentially as if it had those flags and it defaulted to If we are going to allow for manual control over compaction, perhaps we could add those flags, and wire them up appropriately? I will also note that kine will always refuse to compact the most recent 1000 revisions. That is hardcoded and I wouldn't recommend changing it, at the risk of having nodes miss rows from the database in the case of multiple nodes talking to the same sql db. |
@brandond I'm happy to add support for those flags, but supposing they've been added, kine (and etcd) would respect ad hoc compaction requests, right? Other than in the edge case you mentioned. If that's the case, what are your thoughts on merging this and adding the flags in a follow-up PR? I don't mind adding that here either, just trying to keep the scope of this PR as narrow as possible. |
So I just looked at this again and I see that you're adding support for the direct compact grpc method, without modifying the compact transaction that the apiserver sends - the apiserver's automatic compaction should still be a no-op because we always fail the transaction. As long as we're not changing the interaction with the apiserver, I think we're OK. |
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
This PR adds support for gRPC compaction requests for all backends. For NATS, compaction is handled by Jetstream and the bucket's
History
setting, so compaction requests are no-ops.Additionally, unused params were removed from the LimitedServer's
compact
andcreate
methods.Given that the default compaction interval of 5min is not exposed as a configuration option, it can be useful to be able to trigger ad hoc compactions, which partially addresses #230.
Test code
Sample output