Skip to content

Fix subscription commit recovery after RPC timeouts - #19023

Merged
jt2594838 merged 6 commits into
masterfrom
fix/subscription-sync-commit-retry
Oct 27, 2026
Merged

jt2594838 merged 6 commits into
masterfrom
fix/subscription-sync-commit-retry

Conversation

@Caideyipi

@Caideyipi Caideyipi commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Transient subscription commit connection failures currently escape from commitSync after provider recovery is scheduled. A consumer that stops on that exception can stop making progress even though the provider can recover.

Explicit synchronous and asynchronous ACKs now retry only unconfirmed contexts: the initial attempt plus at most three retries, five seconds apart. Connection failures and partial acceptance share that budget. If a response accepts 8 of 10 contexts, those exact 8 leave the pending buffer immediately; only the other 2 are retried. Successful DataNode groups and RPC batches are preserved. Each response's checkpoint is applied once, so completing an older retry cannot reapply its checkpoint over a newer concurrent commit. Waiting happens outside the provider read lock; server errors, close, fencing, and interruption stop retries.

Automatic/close-time commits and redirected ACK flushes also preserve completed-batch results when a later batch loses its connection, removing accepted contexts before their existing subsequent flushes. File/tablet continuation-poll cleanup retains the original recoverable poll failure if its NACK encounters a connection failure. NACKs are sent once because replaying them can increment the server's poison-message counter again; fencing and server errors still propagate.

Validation on a remote JDK 17 host:

  • All four new regressions for partial acceptance and automatic/redirected ACK progress reproduced the old behavior before the fix.
  • A clean 11-module IoTDB dependency reactor, two Maven threads, sanitized Maven settings, and disabled build caches passed 57 tests, 0 failures, 0 errors, 0 skipped. Checkstyle and Spotless passed.
  • Required TsFile common and tsfile artifacts were rebuilt from source commit eb733133b9eee0b1eb74f7671d809ecd730cedc3; only Maven coordinates were adapted to 2.5.1-261023-SNAPSHOT. Installed JAR hashes matched the source build. The third-party dependency download cache was reused.
mvn -T 2 clean test -pl iotdb-client/subscription -am -P with-integration-tests \
  -Dtest=SubscriptionConsumerMultiProviderPollTest,SubscriptionConsumerLifecycleTest,SubscriptionProviderConnectionTimeoutTest,SubscriptionProviderSlicingTest \
  -Dsurefire.failIfNoSpecifiedTests=false

All newly added test methods are in SubscriptionConsumerMultiProviderPollTest. Local mvn spotless:apply -P with-integration-tests -pl iotdb-client/subscription and git diff --check passed; local unit tests and the original long-running workload were not run.

@Caideyipi

Copy link
Copy Markdown
Collaborator Author

Self-review passed at 72494ee3e40 after reviewing the complete diff and the related commit batching, provider recovery, server ACK handling, and consumer lifecycle paths. All confirmed review findings are fixed.

Fixed [P2] Whole-group retries replay completed RPC batches in AbstractSubscriptionConsumer.java:1588 and AbstractSubscriptionProvider.java:816. A DataNode group can span multiple RPCs. If a later RPC lost its connection, the earlier responses were discarded and the whole group was retried, replaying accepted ACKs, retaining confirmed contexts after retry exhaustion, or hiding an earlier rejection. Completed results now preserve accepted contexts and checkpoint progress; retries send only the unconfirmed suffix, and an earlier rejection prevents automatic retry.

Validation:

  • Added three regression methods covering completed-batch replay, accepted-context/checkpoint retention after exhaustion, and rejection followed by a connection failure. All three failed before the fix and passed afterward.
  • A clean 11-module dependency reactor passed on a remote JDK 17 host with two Maven threads, an isolated Maven repository, sanitized global/user settings, and build-cache reuse disabled. All 50 tests passed, including the original six new methods and these three additions. Checkstyle and Spotless passed. The tested sources match the pushed commit.
  • Local mvn spotless:apply -P with-integration-tests -pl iotdb-client/subscription and git diff --check passed. Unit tests ran remotely; local tests and the original long-running workload remain unrun.

Focused test selection (with the isolated repository and sanitized settings also supplied):

./mvnw -T 2 clean test -pl iotdb-client/subscription -am -P with-integration-tests \
  -Dtest=SubscriptionConsumerMultiProviderPollTest,SubscriptionConsumerLifecycleTest,SubscriptionProviderConnectionTimeoutTest,SubscriptionProviderSlicingTest \
  -Dsurefire.failIfNoSpecifiedTests=false

The original head's SonarCloud quality gate was failing; the 50 GitHub annotations inspected were outside this PR's diff and were left unchanged.

@Caideyipi

Copy link
Copy Markdown
Collaborator Author

Re-review completed after checking the full PR diff and the related commit batching, server ACK handling, provider recovery, asynchronous commit, and consumer lifecycle paths. Fixed one additional issue in 53b3b02ad4b.

[P2] Retry completion could roll back a newer committed checkpoint — AbstractSubscriptionConsumer.java:1589. After an earlier RPC batch succeeded and a later batch lost its connection, the earlier batch's progress was immediately applied. A concurrent ACK could then advance the same region, but the successful retry merged the old snapshot into its result and applied it again, moving the recoverable checkpoint backward. Completed-batch progress is now applied once, and retry completion returns only the final attempt's progress. Accepted-context accounting and retrying only unconfirmed batches are preserved.

Added testAsyncCommitRetryDoesNotOverwriteNewerCommittedProgress, which coordinates an asynchronous retry with a newer synchronous ACK to the same region.

Validation:

  • The final regression test failed against the pre-fix implementation: expected localSeq=2, but retry completion restored localSeq=1. It passes with the fix.
  • On a remote JDK 17 host, the clean 11-module dependency reactor passed with two Maven threads, an isolated Maven repository, sanitized global/user settings, and build caches disabled. All 51 tests passed, with no failures, errors, or skips. Checkstyle and Spotless passed. The tested source blobs match the pushed commit.
  • Local mvn spotless:apply -P with-integration-tests -pl iotdb-client/subscription and git diff --check passed. Unit tests ran remotely; local unit tests and the original long-running workload were not run.

Focused test command, with the isolated repository, sanitized settings, and cache disabling also supplied:

mvn -T 2 clean test -pl iotdb-client/subscription -am -P with-integration-tests \
  -Dtest=SubscriptionConsumerMultiProviderPollTest,SubscriptionConsumerLifecycleTest,SubscriptionProviderConnectionTimeoutTest,SubscriptionProviderSlicingTest \
  -Dsurefire.failIfNoSpecifiedTests=false

@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 110 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.82%. Comparing base (8affcb3) to head (b86ea09).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...on/consumer/base/AbstractSubscriptionConsumer.java 0.00% 97 Missing ⚠️
...on/consumer/base/AbstractSubscriptionProvider.java 0.00% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19023      +/-   ##
============================================
- Coverage     47.83%   47.82%   -0.01%     
  Complexity      604      604              
============================================
  Files          5562     5562              
  Lines        408030   408227     +197     
  Branches      53644    53685      +41     
============================================
+ Hits         195172   195245      +73     
- Misses       212858   212982     +124     

☔ 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.

@Caideyipi

Caideyipi commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

Re-review fixes are in 4bbeedd2952.

  • P2: cleanup NACK failures masked recoverable continuation-poll failures. At AbstractSubscriptionConsumer.java:1066 and line 1275, a cleanup connection failure could replace the original non-critical poll error with a critical exception, interrupting consumption and leaving events awaiting server recycling. Cleanup now preserves the original error and attaches the connection failure as suppressed. NACKs are sent once: replaying them can increment the server's poison-message counter again and eventually force an ACK. Fencing and server errors still propagate.
  • P2: automatic and redirected ACKs discarded earlier successful batches after a later connection failure. At AbstractSubscriptionConsumer.java:1691 and line 2181, accepted contexts stayed pending and their returned checkpoint was not applied. Subsequent flushes replayed those contexts. Both paths now preserve the completed result, apply its checkpoint, and remove only accepted contexts; only the unconfirmed remainder is retained or staged for redirect.
  • Partial ACKs now retry only the rejected remainder. commitWithRetry at line 1580 removes accepted contexts by identity, including non-prefix selections. An 8-of-10 response removes those exact 8 immediately and retries only the other 2. Partial responses and connection failures share the three-retry/five-second budget. Accepted progress is retained after exhaustion and applied once; server errors, close, fencing, and interruption stop retries. Explicit asynchronous ACKs share this path; automatic/redirected ACKs retain their existing subsequent-flush retry mechanism.

Validation:

  • All four new partial-acceptance and automatic/redirected ACK regressions reproduced the old behavior before applying the fix.
  • Remote JDK 17 clean 11-module source reactor: 57 tests passed, 0 failures, 0 errors, 0 skipped, including all SubscriptionConsumerMultiProviderPollTest cases. Build caches were disabled; Checkstyle and Spotless passed. Required TsFile dependencies were source-built from eb733133b9eee0b1eb74f7671d809ecd730cedc3, with only Maven-coordinate adaptation to 2.5.1-261023-SNAPSHOT; installed JAR hashes were verified. The third-party download cache was reused.
  • Tested source files matched the local files byte for byte. Local Spotless and git diff --check passed; local unit tests and the original long-running workload were not run.

@Caideyipi

Copy link
Copy Markdown
Collaborator Author

Re-review of the complete diff at 4bbeedd2952 and the related ACK, continuation-poll, and lifecycle paths is complete. Two additional P2 issues are fixed in b86ea0926b4:

  • Delayed completed-batch results could still roll back a newer concurrent checkpoint — AbstractSubscriptionConsumer.java:1607, with the replacement at line 2257. If one batch succeeds, a later RPC blocks, and another provider commits newer progress before that RPC loses its connection, the completed result is first applied after the newer commit. Applying each result once did not prevent this ordering: the regression moved one writer from sequence 2 back to 1 and discarded another writer's checkpoint. ACK results now merge monotonically per writer, preserving concurrent progress. Explicit seeks retain their existing ability to move backward.
  • A later server error discarded earlier successful ACK batches — AbstractSubscriptionProvider.java:815. Completed results were preserved only for connection exceptions. A subsequent server rejection or fencing response therefore left already accepted contexts pending and omitted their returned checkpoint, allowing subsequent flushes to replay confirmed ACKs. Completed results now survive server exceptions across synchronous, asynchronous, automatic, close-time, and redirected ACKs. The original exception identity/type and consumer fencing are preserved; server errors are not retried. Automatic commits also remove confirmed results before propagating an error that prevents returning their removable-context set.

Validation:

  • All six new regression methods failed against 4bbeedd2952 with the expected context-retention or checkpoint-rollback assertions, then passed with the fix. Server-error coverage includes non-critical errors, critical errors, owner fencing, and consumer fencing across all five ACK paths.
  • Remote JDK 17 clean 11-module source reactor: 63 tests passed, 0 failures, 0 errors, 0 skipped, covering SubscriptionConsumerMultiProviderPollTest, SubscriptionConsumerLifecycleTest, SubscriptionProviderConnectionTimeoutTest, and SubscriptionProviderSlicingTest. A separate clean Chinese-locale test-compile of the same dependency reactor also passed. Checkstyle and Spotless passed.
  • Both builds used two Maven threads, an isolated repository, sanitized global/user settings, and disabled build caches. Required TsFile dependencies were rebuilt from source eb733133b9eee0b1eb74f7671d809ecd730cedc3, with only Maven coordinates adapted to 2.5.1-261023-SNAPSHOT; installed JAR hashes matched the source build. Third-party download caches were reused. Tested source hashes match the pushed commit.
  • Local mvn spotless:apply -P with-integration-tests -pl iotdb-client/subscription and git diff --check passed. Local unit tests and the original long-running workload were not run.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@jt2594838
jt2594838 merged commit 07dc281 into master Oct 27, 2026
40 of 41 checks passed
@jt2594838
jt2594838 deleted the fix/subscription-sync-commit-retry branch October 27, 2026 07:44
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.

2 participants