feat(go): add stream/topic purge API with BDD tests and fix server purge bug - #3608
feat(go): add stream/topic purge API with BDD tests and fix server purge bug#3608chengxilo wants to merge 22 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3608 +/- ##
============================================
- Coverage 85.15% 85.13% -0.02%
Complexity 1402 1402
============================================
Files 1230 1226 -4
Lines 181317 181327 +10
Branches 147611 147607 -4
============================================
- Hits 154392 154376 -16
- Misses 22874 22907 +33
+ Partials 4051 4044 -7
🚀 New features to boost your workflow:
|
|
@chengxilo i'll check this during the weekend. could you please rebase? |
|
I've checked whether there is potential concurrency issue due to interleaving of flush + purge commands as this is what I've reckon from the changes could potentially be an issue and did not find any problems with that. Let's wait for @hubcio review and we can merge it. |
8bc60d8 to
f5d74d9
Compare
|
/ready |
|
nothing has been changed, just because I rebased onto master locally so it was forced pushed() I shouldn't do that tho. |
9b5e79c to
a604653
Compare
|
@chengxilo could you please rebase again? i will review this today |
done. Currently, we get the pre-purge messages back for roughly > a second. I'm not sure if we should justify it in BDD test or fix it. Previously for the non-VSR it was fixed. But not for the VSR enabled one. (since according to the comments, it was intended) |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either Thank you for your contribution! |
|
@chengxilo Seems the rust bdd test is failing? /author |
| And I poll messages from stream "purge-test-stream", topic "purge-topic", partition 0 starting from offset 0 | ||
| Then I should receive 0 messages |
There was a problem hiding this comment.
Could we wait for purge convergence before polling here? The purge command commits a new purge generation, but the partition reconciler clears the messages on a later tick. This immediate poll can therefore still see pre-purge data and make this scenario flaky (faced this while writing tests for cpp). Consider adding bounded retry/wait logic before asserting the message count. The same applies to the other immediate post-purge polls in this feature.
Yes, I am wondering if this is a behavior that need to be fixed. Though in the comment in server side, it seems to be expected. (I mean like... I personally don't like this behavior. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either Thank you for your contribution! |
# Conflicts: # bdd/go/tests/basic_messaging.go # bdd/go/tests/leader_redirection.go
Which issue does this PR address?
Relates to #2148
Rationale
Go SDK doesn't have the purge topic/ purge stream API, so this PR implement them. To test it, I added an new BDD test scenario, and implemented both Go and Rust BDD tests.
What changed?
The Go SDK was missing PurgeStream and PurgeTopic methods. Added them along with a shared BDD scenario (stream_topic_purge.feature) and implementations in both Go and Rust.
While testing, discovered that purge_all_segments only cleared on-disk segments but left stale messages in the journal and in-flight tiers. Polling after purge would return messages that should have been deleted. Fixed by calling
journal.reset()andclear_in_flight()during partition re-initialization. I am not famliar with Rust server code so during the code review this part worth more attentionLocal Execution
AI Usage