SEPURI-SAI-KRISHNA opened a new pull request, #12417:
URL: https://github.com/apache/seatunnel/pull/12417
### Purpose of this pull request
Follow-up to Issue 2 of @DanielLeens's review on #12349. He raised it as
non-blocking and agreed to defer it, and I said twice in public that it would
follow once #12349 merged, on #12349 itself and in the #12393 body. #12349
merged this morning as `b70652be8a`, so here it is.
**The comment.** The `return Collections.emptyMap()` inside the per-topic
loop exits the whole method, so it discards anything `consumerOffsets` has
already collected for topics earlier in the list.
`RocketMqSourceOptions.TOPICS` accepts a comma separated list, so multi-topic
is a supported configuration rather than a hypothetical.
As Daniel and I both concluded, this is not reachable today and is not a
regression, because the retry topic is scoped to the group rather than the
topic: a group cannot commit an offset for any topic without first registering,
and registering is what creates the retry topic. So a missing retry topic
implies nothing was committed for any topic in the list, and the map is
necessarily still empty at that point. The comment records the reasoning and
says what has to change if the invariant ever stops holding, which was the
whole point of his request.
**A test, which was not asked for.** Checking the loop before writing the
comment turned up that all five existing cases in `RocketMqAdminUtilTest` pass
`Collections.singletonList(TOPIC)`. Nothing covered accumulation across topics
at all. A comment can be deleted in a later edit; a test cannot be deleted
quietly, so `testCurrentOffsets_multipleTopicsAreMergedAcrossTheLoop` now pins
it.
I deliberately did not add a test for the discarding behaviour itself. It is
unreachable under the invariant, and asserting today's behaviour would cement
the very thing the comment says should change if the invariant ever breaks.
### Does this PR introduce _any_ user-facing change?
No. The change to `RocketMqAdminUtil` is a comment: the file is
byte-identical to `dev` once comments are stripped. The other file is a test.
### How was this patch tested?
`./mvnw -pl seatunnel-connectors-v2/connector-rocketmq test`: **24 tests, 0
failures, 0 errors**, up from 23. `verify` passes on JDK 11 and
`spotless:check` was run separately with up-to-date caching disabled.
To confirm the new test earns its place, I made the mis-edit the comment
warns about, stopping the loop after the first topic:
```
expected: <{test-topic/broker-a/0=42, other-test-topic/broker-a/0=7}>
but was: <{test-topic/broker-a/0=42}>
```
It fails exactly one test, the new one. All five pre-existing cases stay
green under that mutation, which is the gap this closes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
