SEPURI-SAI-KRISHNA commented on PR #12392:
URL: https://github.com/apache/seatunnel/pull/12392#issuecomment-5749812112
All four addressed. Thank you for these, particularly Issue 3, which is my
own finding from #12332 applied back to my own change and I did not spot it.
**Issue 1.** `shouldThrowException` is now `true` on the `currentOffsets`
retry. You are right about the consequence: `RetryUtils` returns `null` at
`RetryUtils.java:79` once retries are exhausted with `false`, and
`RocketMqIT.java:557` then does `currentOffsets.containsKey(mq)`, so a genuine
failure would have surfaced as a bare NPE with `lastException` discarded.
Reading that method again, `false` costs a second thing you did not mention: at
`RetryUtils.java:51-54` an exception the predicate rejects is swallowed rather
than rethrown, so a non-retriable failure quietly burns the remaining attempts
and also ends at that `null`. `true` fixes both. The comment now says why this
call differs from its neighbour.
I have deliberately not changed the `offsetTopics` retry above it, which has
the same property: exhaustion there returns `null` and `consumer.assign(null)`
follows. That one is pre-existing rather than introduced here, and widening
this PR to cover it felt like the wrong trade. Say the word if you would rather
it went in too.
**Issue 2.** Corrected. The comment claimed `currentOffsets` already raises
`RocketMqConnectorException`, which is only true after #12349. It now says that
on `dev` the call still maps a failed lookup onto an empty map, that the
wrapper is a consistency fix today, and that it becomes load bearing once
#12349 merges.
**Issue 3.** Corrected, though I have reached a different conclusion on the
second half and want to explain rather than quietly diverge.
You are right that the guard confirms this topic's route while
`currentOffsets` resolves `%RETRY%<group>`. I think it still covers that read,
for a reason the old comment did not give: the name server drops routes per
broker rather than per topic, so a resolvable route for the data topic means
the broker registration is live and the retry topic's route with it. The retry
topic exists by that point because the job has already consumed with that
group. The comment now states exactly that, and says plainly that it is a
short-gap guard, since `waitForTopicRoute` gives up after a minute and the
4m24s outage would still fail, only with a route message rather than an opaque
assertion timeout.
On widening `checkOffsetNoDiff` to 60 seconds, I would rather not. Neither
30 nor 60 seconds survives a multi-minute gap, so the widening buys no real
resilience, and keeping the window tight means a genuine offset mismatch still
fails fast. It is also the opposite of the position I argued on #12323 and you
accepted there, that we remove causes rather than widen windows. Happy to be
overruled if you see it differently.
**Issue 4.** `waitForTopicRoute("test_topic_message_tag")` added to
`testSinkRocketMqMessageTag` before the read, matching the sibling sink tests.
Verified locally: `verify` passes on JDK 11, and `spotless:check` separately
with up-to-date caching disabled. The branch is rebased onto current `dev`, so
it now sits on top of #12393, #12395 and #12396.
--
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]
