feat(cogs): ChangeStream trait, preparation for backend instrumentation - #590
feat(cogs): ChangeStream trait, preparation for backend instrumentation#590matt-codecov wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## matth/storage-inventory-tracker #590 +/- ##
===================================================================
- Coverage 93.31% 88.12% -5.19%
===================================================================
Files 10 100 +90
Lines 703 16448 +15745
===================================================================
+ Hits 656 14495 +13839
- Misses 47 1953 +1906
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /// a change stream for our storage backends. | ||
| /// | ||
| /// See [module docs](self). | ||
| pub trait InventorySink: fmt::Debug + Send + Sync + 'static { |
There was a problem hiding this comment.
Calling out that this interface is close to what is being created in #582. I don't have a concrete design in mind yet, but it seems the "inventory" concept would be capable of solving both problems:
- Storage cost tracking, which requires to know which files are stored until when
- GC / eviction, which requires to know which files are stored until when
There was a problem hiding this comment.
Apart from that we don't want to couple the two in-flight PRs too much, so this is probably more relevant for the other PR:
The biggest difference is how they are needed: Cost tracking requires the inventory independent of the backend, and will export it via kafka to a remote system. Conversely, the S3 and FS backends need the inventory as part of their inner workings. Per design, GC is part of the backend's responsibility.
We might be able to separate this cleanly if we:
- Hook the inventory up in the service like the PR here does (sort of like an event bus)
- Allow multiple inventory exporters
- One is the kafka sink used for cost tracking
- Another one is a database sink (like sqlite, PG) for GC, configured/hooked when S3/FS are used
- GC is still an external command then, that queries said database
There was a problem hiding this comment.
my direction here is very much "fail open" and errors are logged and swallowed. if inventory tracking is load-bearing for GC in some backends, that'll need to change
correct me if i'm wrong: for self-hosted, in order to support the automatic TTI/TTL GC that we advertise, we have to implement it ourselves atop filesystem and S3 as we anticipate those will be popular in self-hosted deployments?
94e1e3a to
f819003
Compare
matt-codecov
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f819003. Configure here.
| /// a change stream for our storage backends. | ||
| /// | ||
| /// See [module docs](self). | ||
| pub trait ChangeStream: fmt::Debug + Send + Sync + 'static { |
There was a problem hiding this comment.
New change stream module never compiled
Medium Severity
backend/mod.rs has no change_stream declaration, so this file is not part of the crate. Nothing in it is type-checked, ChangeStream/NoopStream/ProducerStream are unreachable from other modules, and the unit tests here never run — silently leaving the newly added objectstore-inventory-tracker dependency unused until a follow-up wires it in.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f819003. Configure here.
There was a problem hiding this comment.
oops lol artifact of commit splitting
3 participants
Depends on #588
ChangeStreamtrait wrapsInventoryTrackerto log/count/swallow errors and allow aNoopStreamto be plugged in for tests to avoid pulling in Kafka stuff.