Skip to content

Added support of RetryConfig topic readers - #722

Open
alex268 wants to merge 2 commits into
ydb-platform:masterfrom
alex268:master
Open

Added support of RetryConfig topic readers#722
alex268 wants to merge 2 commits into
ydb-platform:masterfrom
alex268:master

Conversation

@alex268

@alex268 alex268 commented Sep 10, 2026

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.22222% with 152 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.30%. Comparing base (12b9fcf) to head (d4edff1).

Files with missing lines Patch % Lines
...main/java/tech/ydb/topic/read/impl/ReaderImpl.java 39.23% 70 Missing and 9 partials ⚠️
...ain/java/tech/ydb/topic/read/impl/ReadSession.java 73.94% 19 Missing and 12 partials ⚠️
...java/tech/ydb/topic/read/impl/AsyncReaderImpl.java 74.35% 19 Missing and 1 partial ⚠️
.../java/tech/ydb/topic/read/impl/SyncReaderImpl.java 86.48% 4 Missing and 6 partials ⚠️
...tech/ydb/topic/read/impl/MessageCommitterImpl.java 52.94% 6 Missing and 2 partials ⚠️
...n/java/tech/ydb/topic/settings/ReaderSettings.java 42.85% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #722      +/-   ##
============================================
+ Coverage     73.09%   73.30%   +0.21%     
+ Complexity     3578     3541      -37     
============================================
  Files           392      391       -1     
  Lines         16548    16440     -108     
  Branches       1736     1723      -13     
============================================
- Hits          12096    12052      -44     
+ Misses         3826     3771      -55     
+ Partials        626      617       -9     

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

@KirillKurdyukov KirillKurdyukov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found several lifecycle and retry regressions that should be fixed before merge:

  1. ReaderImpl never stores or invokes ReaderSettings.getErrorsHandler(). Both retryable and terminal failures are only logged, contradicting the new NEVER documentation and regressing the previous retrier behavior.
  2. AsyncReaderImpl.shutdown() and SyncReaderImpl.shutdown() ignore a false result from impl.close(). Before init or while waiting for a scheduled retry there is no active stream, so onClose is never called: the async shutdown future stays pending and sync shutdown blocks forever.
  3. A terminal error before the first InitResponse never completes initFuture. With NEVER or STANDARD, AsyncReader.init() remains pending and SyncReader.initAndWait() hangs.
  4. ReadSession.closeAll() never sets isClosed = true. Committing an already delivered message after reconnect creates a future, reports the send as accepted, then silently drops the request because the underlying stream is closed. That future can never complete.
  5. In ReadSession.onCommitOffset(), encountering one unknown partition executes return from the whole method. A response may contain several partitions, so acknowledgements and commit futures for later active partitions are skipped. This needs continue.
  6. Exceptions from user callbacks now go through impl.fail(CLIENT_INTERNAL_ERROR). The default TopicRetryConfig.FOREVER retries that status, so a deterministic callback failure can produce an endless reconnect/redelivery loop; the old implementation stopped the reader.
  7. onSessionStarted and onReaderClosed bypass the configured handler executor and serial control-event queue. This changes thread affinity and allows onReaderClosed to overtake queued partition-closed events.
  8. SyncReaderImpl.handleReaderClosed() never closes the default decompression LazyExecutor. After a compressed message it leaks non-daemon pool threads and can keep the JVM alive.
  9. Terminal stream closure does not signal waitingCondition, so a thread in receive() with a long explicit timeout remains blocked until that timeout expires.
  10. A graceful stop request for an unknown partition is now only logged and ignored. The previous implementation restarted the stream; the new behavior leaves the server waiting for a response and the protocol state inconsistent.

The existing test suite passes, but this PR adds no retry/lifecycle tests. Codecov patch coverage is 66.22% with 152 changed lines uncovered. Please add targeted tests for terminal-before-init, shutdown-during-backoff, stale-message commit, multi-partition commit acknowledgements, callback failure, and executor cleanup.

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