SEPURI-SAI-KRISHNA commented on PR #12393:
URL: https://github.com/apache/seatunnel/pull/12393#issuecomment-5740950176
Thank you for this, and for saying clearly which parts you could verify and
which you could not. That distinction is what made the review useful rather
than just directional.
I have taken the PR in a different direction as a result: the helper is now
**removed** rather than repaired. The reasoning is in the updated description;
the short version is that your Issue 4 turned out to be decisive, and it
decides against fixing the method at all.
**Issues 1 and 2: both confirmed, and both mine.**
Issue 1 is the one I am least happy about. Earlier in this series, while
investigating #12332, I decompiled `getTopicRouteInfoFromNameServer` and
established that it always throws rather than returning null, and I wrote
exactly that into a comment on #12332. Then I wrote a null guard against it
here anyway. You are right that the guard is dead and that the first call for
each topic would have thrown.
Issue 2 I had not checked, and your reading is correct.
`fetchNameServerAddr()` returns the field `nameSrvAddr`, which is assigned only
inside the top-addressing success path; `setNamesrvAddr` writes
`ClientConfig.namesrvAddr` and reaches the remoting client through
`MQClientInstance.updateNameServerAddressList`, never that field. On a runner
that cannot reach the top-addressing service it returns null and
`ns.split(";")` throws. My comment claiming the client would "resolve the name
server list itself" described behaviour the library does not have.
**Issue 4: confirmed, and it is why the method is gone.**
You said you could not determine which way this goes. It goes badly, and the
broker jar settles it. `AdminBrokerProcessor.deleteTopic` calls
`TopicConfigManager.deleteTopicConfig`, `MessageStore.cleanUnusedTopic` and,
under `autoDeleteUnusedStats`, `BrokerStatsManager.onTopicDeleted`. There is no
`ConsumerOffsetManager` call in that handler, so committed offsets survive a
topic delete.
Combined with `rocketMqContainer` being created in `@BeforeAll` and shared
across every engine leg, a working delete gives
`testSourceRocketMqTextTagToConsole` a topic reset to offset 0 while the
group's committed offset stays at 32, so the second leg onward reads nothing
against a 32-row assertion. That test passes today *because* the cleanup never
happens.
**One correction to the review.** You describe both confs as `MIN_ROW =
MAX_ROW = 32`. `rocketmq-source_text_error_tag_to_console.conf` is `MIN_ROW =
MAX_ROW = 0`, which is the point of that test: the tag filter matches nothing.
So only `testSourceRocketMqTextTagToConsole` would have broken. That narrows
the blast radius to one test but does not change the conclusion.
**Issue 3: I think this one is already covered, and I would rather not add a
redundant call.** `generateTestData` calls `waitForTopicRoute(topic)` at line
453 before any send, and that helper both recreates the topic through
`producer.createTopic` and asserts the route is visible through
`RocketMqAdminUtil.offsetTopics`, which is the fresh-admin path the connector
itself uses. #12323 placed it there deliberately so that all nine call sites
are covered rather than each test separately. Since `executeJob` runs after
`generateTestData`, the route is confirmed before submission. If you still see
a gap I have missed, say so and I will add it.
**Issue 5** is resolved by the comments going away with the method.
**On the null cluster name**, where you noted the name server module is not
pinned by this repo and so you could not check: I pulled
`rocketmq-namesrv:4.9.4` from Maven Central.
`DefaultRequestProcessor.deleteTopicInNamesrv` tests the cluster name for null
and then for empty, and falls through to the global
`RouteInfoManager.deleteTopic(topic)` in both cases. It is moot now, but it may
be useful to you elsewhere, and you were right to treat my original wording as
an assertion I had not earned.
**Options I considered before removing it**, since this is a reversal rather
than a refinement:
1. Fix the helper and also reset the group's offsets after recreating the
topic. Correct in principle, but it puts per-queue offset manipulation into a
test cleanup helper and deepens its dependence on admin semantics that two
earlier versions of this PR already got wrong.
2. Remove it. Behaviour preserving in the strict sense, because the helper
is a proven no-op: 14 logged failures and zero successes across three job logs.
The callers then run exactly as they do today.
3. Close this and file an issue. Leaves misleading code in the tree for the
next person, who would likely "fix" it the way I first did.
I took 2. It is the only one that is both an improvement and provably zero
risk.
What it does not do is give those two tests the isolation the helper's name
promised. They have never had it. If that is wanted, a dedicated consumer group
per topic in the two confs is the honest way, and I am glad to open it
separately, but it changes test behaviour and does not belong in a patch that
deletes dead code.
--
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]
