Skip to content

[Subscription] Advance consensus progress when discarding poisoned events - #18838

Open
Caideyipi wants to merge 1 commit into
masterfrom
codex/consensus-poison-frontier
Open

Caideyipi wants to merge 1 commit into
masterfrom
codex/consensus-poison-frontier

Conversation

@Caideyipi

Copy link
Copy Markdown
Collaborator

Description

When a consensus subscription event reaches the existing 10-NACK poison threshold, the queue force-ACKs and disposes of it without committing its outstanding writer-progress mapping. Later ACKs for that writer cannot advance the contiguous checkpoint past the discarded event. This can hold back WAL reclamation based on committed progress and cause already handled events to be replayed after recovery.

Commit poisoned events through the ordinary commitAndRefreshWalRetention path before disposing of them in explicit NACK, silent client NACK, and timeout recycling. If the commit is rejected, requeue the event and retain its tablet memory for retry. The existing poison-event policy and threshold are preserved.

Validation

  • The four regression cases fail against the original implementation: all three discard paths leave committed progress unset, and a rejected commit still discards the event.
  • All 71 tests pass in ConsensusPrefetchingQueueTest, ConsensusSubscriptionCommitStateTest, and ConsensusPrefetchingQueueSeekTest. The new cases also verify that a later data event remains committable and that retained memory is released only after a successful commit.
  • Full reactor mvn -Ddevelocity.off=true -nsu test-compile -DskipTests and the same command with -P with-zh-locale: both passed.
  • Spotless, Checkstyle, and git diff --check: passed.
mvn -Ddevelocity.off=true -nsu -pl iotdb-core/datanode -am test \
  -Dtest=ConsensusPrefetchingQueueTest,ConsensusSubscriptionCommitStateTest,ConsensusPrefetchingQueueSeekTest \
  -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false \
  -Diotdb.test.skip=false -Diotdb.ut.skip=false

This PR has:

  • been self-reviewed, including queue locking and per-event ACK/NACK serialization.
  • added a comment explaining why the outstanding mapping must be consumed before disposal.
  • added unit tests for all three discard paths and rejected-commit retry.

Key changed classes: ConsensusPrefetchingQueue, ConsensusPrefetchingQueueTest.

@codecov

codecov Bot commented Oct 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.92%. Comparing base (932c1da) to head (82e3dbc).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...on/broker/consensus/ConsensusPrefetchingQueue.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18838      +/-   ##
============================================
+ Coverage     45.91%   45.92%   +0.01%     
  Complexity      712      712              
============================================
  Files          5502     5502              
  Lines        398775   398778       +3     
  Branches      51967    51967              
============================================
+ Hits         183080   183129      +49     
+ Misses       215695   215649      -46     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant