SEPURI-SAI-KRISHNA commented on PR #12438:
URL: https://github.com/apache/seatunnel/pull/12438#issuecomment-5865774547
Thanks @SEZ9, both findings are right and I have taken both.
**Issue 1.** You are correct that the wording contradicted itself. It said
the transition "would reach this return with offsets already collected" and
then hedged with "If that ever becomes reachable". I checked the mechanism
against the code rather than just accepting the wording was awkward, and the
reachability argument holds:
- `topicRouteAvailable(adminClient, topic)` at
`RocketMqAdminUtil.java:388-397` calls `examineTopicRouteInfo(topic)` on the
**requested** topic. It never probes the group's retry topic, so the Javadoc's
per-broker argument only holds when the two live on the same broker.
- `RocketMqSourceSplitEnumerator.java:380-383` is `if
(groupOffsets.isEmpty()) { listOffsets(queues, CONSUME_FROM_FIRST_OFFSET) }`,
so an empty map is a full rewind of every topic, exactly as you describe.
So on a multi-broker layout the discard is reachable and the replay hazard
is real. The comment now says that plainly instead of hedging:
> The invariant does not cover one transition, and that transition is
reachable today. The guard above probes the requested topic, not the group's
retry topic, and every iteration re-resolves the retry topic. On a multi-broker
cluster where the retry topic and a later topic in the list are hosted on
different brokers, losing the retry topic's broker mid-loop leaves the later
topic resolvable, so the guard passes and this return discards the offsets
already collected for the earlier topics. The caller reads an empty map as a
cold start and rewinds every topic to its first offset, so supporting that
layout means turning this into a continue that keeps the earlier offsets.
On actually making it a `continue` plus the two-topic
`RocketMqAdminUtilTest` case: I would rather not fold that in here. This PR is
comment-only by construction (stripping comments leaves it byte-identical to
`dev`), and that change alters production behaviour on a path with a replay
hazard, so it deserves its own PR, its own issue and a reviewer looking at the
behaviour rather than at wording. I am happy to open it right after this lands,
with the two-topic mock next to
`testCurrentOffsets_multipleTopicsAreMergedAcrossTheLoop` as you specified. If
you would rather see them together, say so and I will close this and file the
combined one instead.
**Issue 2.** Agreed, and agreed on the stronger version. A comment anchored
on a PR number and on what the code used to be is the same pattern that went
stale and created this PR, so I dropped the history rather than updating it.
You are also right that "instead of returning an empty map" was imprecise: the
cold-start branch at `RocketMqAdminUtil.java:361` still returns
`Collections.emptyMap()`, and the `getMinOffset()` fallback at
`RocketMqIT.java:557-559` depends on it. The comment now states only the
present contract and points at that fallback:
> currentOffsets raises RocketMqConnectorException when the lookup itself
fails, and returns an empty map only when the group has legitimately committed
nothing, so matching that exception here retries transient metadata failures
without masking a genuine cold start. The empty-map case is what the
getMinOffset() fallback below covers.
Still comment-only, no behaviour change.
--
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]
