-
Notifications
You must be signed in to change notification settings - Fork 45
Periodic sqlite vacuuming for lq queue #443
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #443 +/- ##
==========================================
+ Coverage 54.69% 54.76% +0.07%
==========================================
Files 120 121 +1
Lines 7345 7363 +18
==========================================
+ Hits 4017 4032 +15
- Misses 3004 3006 +2
- Partials 324 325 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements periodic SQLite vacuuming for the LQ queue to free reusable space, executing VACUUM;
every ten minutes. Additionally, it refactors the wait group management and logging infrastructure for e2e tests.
- Adds a new
vacuumer
goroutine that runs VACUUM on SQLite database every 10 minutes - Migrates from manual wait group management to errgroup pattern for better goroutine lifecycle management
- Replaces Unix socket logging with in-memory pipe connections for e2e tests
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/pkg/source/lq/vacuumer.go | New vacuumer implementation that periodically runs VACUUM on SQLite database |
internal/pkg/source/lq/lq.go | Updates to use errgroup and start the vacuumer goroutine |
internal/pkg/source/lq/producer.go | Removes manual wg.Done() call for errgroup compatibility |
internal/pkg/source/lq/finisher.go | Removes manual wg.Done() call for errgroup compatibility |
internal/pkg/source/lq/consumer.go | Removes manual wg.Done() call for errgroup compatibility |
internal/pkg/log/log.go | Updates logging shutdown to handle new e2e connection pattern |
internal/pkg/log/config.go | Replaces Unix socket logging with pipe-based e2e logging |
internal/pkg/controler/signal.go | Exposes signal channel for e2e test integration |
internal/pkg/config/config.go | Updates config fields for new e2e logging approach |
e2e/test/lqvacuumer/lq_vacuumer_test.go | New test to verify vacuumer functionality |
e2e/test/lqvacuumer/config.toml | Configuration for vacuumer test |
e2e/e2e.go | Updates e2e framework to use pipe connections instead of Unix sockets |
cmd/cmd.go | Updates CLI flags for new e2e logging pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if err != nil { | ||
logger.Error("vacuuming failed", err) | ||
} | ||
logger.Info("vacuuming complete") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VACUUM operation can be resource-intensive and may block other database operations. Consider using 'PRAGMA incremental_vacuum;' for less disruptive maintenance or implementing a check to avoid vacuuming when the database is under heavy load.
logger.Info("vacuuming complete") | |
logger.Info("incremental vacuuming") | |
_, err := s.client.dbWrite.Exec("PRAGMA incremental_vacuum;") | |
if err != nil { | |
logger.Error("incremental vacuuming failed", err) | |
} | |
logger.Info("incremental vacuuming complete") |
Copilot uses AI. Check for mistakes.
As I expected and as Copilot confirmed, VACUUM does block the operations of the DB, I don't think it should be used like that. Although maybe a trigger of VACUUM when shutting down Zeno? It might take a lot of time though so.. not sure. Maybe Copilot's suggestion is enough. |
Incremental vacuum likely makes more sense on a 10 minute basis. I like Corentin's idea of a full VACUUM on shutdown (or maybe bootup or both?) |
367d47a
to
4d60767
Compare
Execute
VACUUM;
every ten minutes to free LQ sqlite reusable space.#94.
the branch is based on #441, pls merge it before.