[Improve][Connector-V2] Correct two RocketMQ comments that no longer hold - #12438
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
…hold Both comments are mine and both stopped being accurate after the PRs they were written alongside merged. No code changes; each file is byte-identical to dev once comments are stripped. RocketMqAdminUtil.currentOffsets said the accumulated map "is necessarily still empty" at the early return. @DanielLeens pointed out on apache#12417 that the invariant does not establish that. Each iteration calls examineConsumeStats again, which re-resolves the group's retry topic, so a retry topic that resolved on an earlier iteration and is gone on a later one reaches the return with offsets already collected. The comment now states the invariant, names that transition as the case it does not cover, and keeps the existing guidance that the return has to become a continue if it ever becomes reachable. RocketMqIT said "On dev currentOffsets still maps a failed lookup onto an empty map, so today this is a consistency fix; it becomes load bearing once apache#12349 makes that call raise RocketMqConnectorException". apache#12349 merged on 2026-09-21, so both halves are now false: the call already raises, and the retry is already load bearing. The comment is rewritten in the present tense. Verified: connector-rocketmq 24 tests pass with spotless:check green, connector-rocketmq-e2e test-compiles, and comment-stripped diffs against dev are empty for both files.
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
What Problem Does This PR Solve?
- User pain point: two comments in the RocketMQ connector had drifted out of sync with the code and history they describe, so a future reader would draw the wrong conclusion from them.
- Fix approach: reword both comments to state the current, accurate situation instead of the stale one, with no behavior change.
- One-sentence summary: this is a comment-only correction that keeps two RocketMQ comments truthful after #12349 landed and after further analysis of the multi-topic early-return invariant in #12417.
1. Code Change Review
1.1 Core Logic Analysis
File 1: seatunnel-connectors-v2/connector-rocketmq/.../common/RocketMqAdminUtil.java, inside the package-private currentOffsets(DefaultMQAdminExt, String, List, Set) overload, in the TOPIC_NOT_EXIST branch that returns Collections.emptyMap().
Before:
// is necessarily still empty here. If that ever stops holding, this has to
After:
// is still empty here. The invariant does not cover one transition: each
// iteration re-resolves the retry topic, so a route that was present on an
// earlier iteration and gone on a later one would reach this return with
// offsets already collected. If that ever becomes reachable, this has to
I traced the loop this comment documents: for (String topic : topics) { adminClient.examineConsumeStats(groupId, topic); ... }, with consumerOffsets accumulated across iterations and discarded via return Collections.emptyMap() on the TOPIC_NOT_EXIST + topicRouteAvailable branch. The original wording ("necessarily still empty") asserted the map is always empty at that return point, which overstates the invariant: the retry-topic existence check (examineConsumeStats) is re-evaluated fresh on every iteration, so if a topic in the middle of the list triggers this branch after an earlier topic already populated consumerOffsets, the map would not be empty when it gets discarded. The new wording correctly narrows the claim to "still empty here" (true for the currently-reachable case, since a missing retry topic implies nothing was ever committed for any topic) while explicitly flagging the one transition the invariant doesn't cover. This is a strictly more accurate statement, not a behavior change - the code path is untouched.
File 2: seatunnel-e2e/.../RocketMqIT.java, comment above the retry-wrapped RocketMqAdminUtil.currentOffsets(...) call in getRocketMqConsumerData.
Before (paraphrased): "On dev, currentOffsets still maps a failed lookup onto an empty map, so today this is a consistency fix; it becomes load bearing once #12349 makes that call raise RocketMqConnectorException."
After: "#12349 made currentOffsets raise RocketMqConnectorException on a failed lookup instead of returning an empty map, and that is the exception this predicate matches, so the retry is load bearing rather than the consistency fix it was when this was written."
I verified against dev that PR #12349 ("Surface RocketMQ group-offset lookup failures instead of rewinding") is already merged (commit b70652be8af, dated 2026-09-21, one day before this PR), and that the public currentOffsets(RocketMqBaseConfiguration, ...) wrapper in RocketMqAdminUtil.java does catch MQClientException and rethrow RocketMqConnectorException - matching the retry predicate exception -> exception instanceof RocketMqConnectorException used at this call site. The old comment's future-tense framing ("it becomes load bearing once #12349...") was stale the moment #12349 merged; the new comment correctly states this in the past tense as already-landed fact. Accurate fix.
Key findings:
- Both hunks are comment-only;
git diff --statconfirms only comment lines changed in both files, zero lines of executable code touched. - Both corrections are independently verifiable against the current
devhistory and the surrounding code, and both are accurate improvements over the previous text. - The RocketMqAdminUtil.java comment now correctly scopes its claim and documents an edge case (mid-loop route loss) that the code does not currently guard against, which is valuable forward-looking context for whoever touches this method next.
- The RocketMqIT.java comment now correctly reflects that the retry wrapper around
currentOffsetsis load-bearing today (not a "future" concern), since the exception it depends on is already being thrown ondev. - No runtime path, config, serialization, checkpoint, or test assertion is touched, so there is no functional or compatibility risk to analyze beyond confirming the comments are truthful.
No runtime-path/state-machine/mermaid diagram is needed here since the change is confined to comments and does not touch seatunnel-engine, checkpoint flow, serialization, or any executable branch.
1.2 Compatibility Impact
Fully compatible. No code, config option, protocol, or public API is modified - only two Java comments. There is no behavioral, serialization, or upgrade-path impact whatsoever.
1.3 Performance / Side-Effect Analysis
None. No executable statement changed.
1.4 Error Handling and Logging
No issues to report; no error-handling or logging code is touched by this diff. No formal Issues raised.
2. Code Quality Assessment
2.1 Coding Standards
No new methods or fields are introduced, so the "core method / nontrivial field must have a comment" check does not apply. The comment wording itself is precise, technically grounded, and improves on the prior text rather than just restating what the code does - it explains the why and the boundary of the invariant, consistent with the project's comment-quality expectations.
2.2 Test Coverage and Test Stability
No test logic, assertions, or control flow are changed in RocketMqIT.java - only a comment above an existing retry-wrapped call. Stability rating: Stable (no risk introduced; nothing to regress). No new tests are required for a comment-only change.
2.3 Documentation Updates
Not applicable - this PR is itself a documentation/comment correction; there is no user-facing docs/en / docs/zh content affected since nothing here is a config option, CLI behavior, or connector capability.
3. Architectural Soundness
3.1 Elegance of the Solution
Precise fix. The scope is exactly right: correct two comments that had gone stale as a direct, traceable consequence of two other merged PRs (#12349, #12417), nothing more and nothing less.
3.2 Maintainability
Improves maintainability: future readers of currentOffsets get an accurate, appropriately-scoped invariant statement instead of an overstated one, and the E2E test comment now correctly signals that the retry wrapper is load-bearing rather than vestigial.
3.3 Extensibility
Not applicable - no structural change.
3.4 Historical-Version Compatibility
Not applicable - comment-only change, no serialization/state/config touched, nothing to break across versions.
4. Issue Summary
No High or Medium issues found. No formal Issues raised for this PR.
5. Merge Recommendation
Conclusion: Ready to merge
- Blockers - none.
- Recommended fixes - none.
This is a small, well-reasoned comment-accuracy fix from an experienced contributor to this connector (author of the referenced #12349 and several other recent RocketMQ/E2E hardening PRs). I independently verified both corrected claims against the current dev history and the surrounding code and found them accurate. No other reviewer has commented on this PR yet.
CI note: the Apache-side "Build" check is still in progress/pending, but the real signal lives on the contributor's fork. The fork's Build workflow run for this head SHA failed, but the only failing job is unit-test (11, windows-latest), and the only failing module is connector-http-paypal (Windows loopback-HTTP timing test), which is unrelated to this PR's RocketMQ comment-only diff and is a known, currently-unresolved Windows timing flake affecting unrelated PRs. This PR does not touch connector-http-paypal in any way, so the failure should not block merge on its own merits; once CI settles (or the flaky job is rerun), this should be green from this PR's own perspective. Recommend merging once CI is confirmed clean (via rerun of the flaky job if needed), no code changes required.
|
The red badge here is not reachable from this diff, and rerunning it is not going to clear it. One leg fails, This PR changes two comment lines in I reran it once and it failed again, so I stopped rather than keep rolling. Measured across three of my PRs on 09-21 and 09-22, that leg fails on this exact assertion in 4 of 6 attempts, which is a two-thirds failure rate rather than an occasional flake. The details are on #12381, which is the open fix: it gives the arrival wait its own 60 second budget while leaving the bounds that test client behaviour at 15 seconds, so a genuine hang still fails fast. It has had @DanielLeens's approval since 09-18 and the diff has not changed since. So there are two ways forward and I am happy with either. Merging #12381 first fixes this for everyone and this PR goes green on its own. Otherwise this one can be taken with the red leg, since the failure is in another module and the diff here is comment-only: both files are byte-identical to |
|
Thanks for the detailed breakdown — I re-checked this independently rather than taking it on faith, and it holds up.
On the flaky-test history: I confirmed For this PR specifically: my "Ready to merge" conclusion from the earlier review stands unchanged. I'd only revise one thing in that review — I'd previously suggested "rerun the flaky job," but given your data that's very unlikely to help before |
This branch has not been deployed
2 participants
Purpose of this pull request
Two comments in the RocketMQ connector no longer describe the code around them. Both are mine, and both went stale when the PRs they referenced merged. This corrects them and changes nothing else.
1.
RocketMqAdminUtil.currentOffsetsoverstates its own invariant.The comment at the
TOPIC_NOT_EXISTearly return says the accumulated map "is necessarily still empty here". @DanielLeens flagged on #12417 that the invariant does not establish that, and he is right. Each loop iteration callsexamineConsumeStatsagain, which re-resolves the group's retry topic. A retry topic that resolved on an earlier iteration and has gone by a later one would reach this return with offsets already collected for the earlier topics.The rewritten comment keeps the invariant and the existing guidance that the
returnhas to become acontinueif that ever becomes reachable, and names the transition it does not cover instead of claiming there is none.2.
RocketMqITdescribes #12349 as unmerged.The comment reads:
#12349 merged on 2026-09-21. Both halves are now false:
currentOffsetsalready raises, and the retry predicate is already load bearing rather than a consistency fix. Rewritten in the present tense.Does this PR introduce any user-facing change?
No. Comments only. Both files are byte-identical to
devonce//comments and blank lines are stripped:How was this patch tested?
spotless:checkis green, andconnector-rocketmq-e2etest-compiles. The container-backedRocketMqITwas not run locally, since Docker is not available in my environment, but the change to that file is a comment.Note on overlapping work
#12350 also touches
RocketMqIT.java. Its hunks run from line 487 to 651 and do not include the comment changed here, so there is no textual overlap.