Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660b2c9d25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.3 #2809 +/- ##
================================================
+ Coverage 45.17% 46.15% +0.98%
================================================
Files 853 1144 +291
Lines 70076 98227 +28151
================================================
+ Hits 31654 45336 +13682
- Misses 35760 49001 +13241
- Partials 2662 3890 +1228
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sei-db/sc/memiavl/db.go
Outdated
| snapshotWriterPool *pond.WorkerPool | ||
|
|
||
| // closed indicates the DB is shutting down | ||
| closed uint32 |
There was a problem hiding this comment.
Can we rename this to pruningInProgress?
blindchaser
force-pushed
the
yiren/v631-prune-fix
branch
from
660b2c9 to
057cec0
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| defer db.pruningInProgress.Store(false) | ||
|
|
||
| startTime := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
As is this is fine to test the theories discussed in the sync.But for a deeper fix i would like to understand the edge cases this change rises.
| db.pruneSnapshotLock.Lock() | ||
| defer db.pruneSnapshotLock.Unlock() | ||
| // CAS: only one prune can run at a time | ||
| if !db.pruningInProgress.CompareAndSwap(false, true) { |
There was a problem hiding this comment.
What if a prune takes just long enough to overlap with the next prune?
In that case the new prune cycle will be skipped and we end up with 2 cycles of unpruned state in the next prune
There was a problem hiding this comment.
yup, looking into the code. we will need to optimize some pruning logic and where/when pruning happens. creating a ticket for that.
There was a problem hiding this comment.
Each prune should prune all snapshots that need to be pruned, not just delete one snapshot. We also should make the pruning logic to be triggered after each commit, instead of only after snapshot creation
4 participants
Describe your changes and provide context
Testing performed to validate your change