Fixed topic's direct write implementation#665
Conversation
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | Medium: Missing
hasPartitionLocation()check inlookupLocation—WriteStreamDirectFactory.java:94-96 - Minor | Medium: Duplicate init-request field logic between
createNewStream()andbuildInitRequest()—WriteStreamDirectFactory.java:55-66 - Minor | Medium: Parameter
partitionIdshadows inherited field inlookupLocation—WriteStreamDirectFactory.java:81 - Minor | Low: Inconsistent visibility —
lookupLocationisprotectedbutlookupPartitionIdisprivate—WriteStreamDirectFactory.java:81,105 - Nit | High: Double blank line —
WriteStreamDirectFactory.java:48-49
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
|
Analysis performed by claude, claude-opus-4-6. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #665 +/- ##
============================================
+ Coverage 71.58% 71.65% +0.06%
- Complexity 3413 3425 +12
============================================
Files 385 386 +1
Lines 16064 16066 +2
Branches 1686 1687 +1
============================================
+ Hits 11500 11512 +12
+ Misses 3909 3901 -8
+ Partials 655 653 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@alex268 Please set up your personal token, see instructions: https://nda.ya.ru/t/dur-L1y77biDC2 |
There was a problem hiding this comment.
Pull request overview
This PR refactors topic write stream initialization to make the init request part of the stream instance (instead of being passed at start() time) and reworks “direct write” into a dedicated WriteStreamDirectFactory, with accompanying test updates and new direct-write coverage.
Changes:
- Refactored
TopicStream/TopicStreamBaseAPI to removestart(initReq, handler)and instead send a constructor-supplied init request onstart(handler). - Split direct-write logic out of
WriteStreamFactoryinto a newWriteStreamDirectFactory(including partition generation + preferred node selection). - Updated and added unit/integration tests covering direct-write scenarios and invalid partitions.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamFactory.java | Simplifies non-direct init request building and stream creation; removes embedded direct-write logic. |
| topic/src/main/java/tech/ydb/topic/write/impl/WriteStreamDirectFactory.java | New implementation for direct-write stream creation (probe by producer, describe topic, preferred node routing). |
| topic/src/main/java/tech/ydb/topic/write/impl/WriteStream.java | Updates stream construction to pass init request into TopicStreamBase. |
| topic/src/main/java/tech/ydb/topic/write/impl/WriteSession.java | Adjusts session to accept a WriteStreamFactory and aligns with new stream start signature. |
| topic/src/main/java/tech/ydb/topic/write/impl/WriterImpl.java | Routes direct-write via WriteStreamDirectFactory and allows injecting a factory for construction. |
| topic/src/main/java/tech/ydb/topic/impl/TopicStream.java | API change: start no longer accepts an init request argument. |
| topic/src/main/java/tech/ydb/topic/impl/TopicStreamBase.java | Stores and sends init request internally; updates start signature accordingly. |
| topic/src/main/java/tech/ydb/topic/impl/TopicStreamFail.java | Updates start signature to match the interface. |
| topic/src/main/java/tech/ydb/topic/impl/TopicRetryableStream.java | Removes init-request plumbing; starts streams with handler only. |
| topic/src/test/java/tech/ydb/topic/write/impl/WriteStreamFactoryTest.java | Updates tests for new WriteStreamFactory constructor and init-request builder method. |
| topic/src/test/java/tech/ydb/topic/write/impl/WriteStreamDirectFactoryTest.java | New comprehensive unit test suite for direct-write factory behavior. |
| topic/src/test/java/tech/ydb/topic/TopicWritersIntegrationTest.java | Adds integration coverage for invalid direct-write partition and fixes topic partitioning settings for determinism. |
| topic/src/test/java/tech/ydb/topic/impl/TopicStreamTest.java | Updates tests to new TopicStreamBase ctor + start(handler) API. |
| topic/src/test/java/tech/ydb/topic/impl/TopicRetryableStreamTest.java | Updates tests to new start(handler) API and init-request-in-stream pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.