DanielLeens commented on PR #11576:
URL: https://github.com/apache/seatunnel/pull/11576#issuecomment-5658379859
Status-update check-in on the new commit since my last review.
**What changed**: the only new commit (`04e29b25f4`) is `Merge
remote-tracking branch 'upstream/dev' into pr-11576`. I want to flag one thing
up front: this is *not* a trivially empty merge like a typical dev-sync — `dev`
picked up `#12244` ("Add protobuf format support for RabbitMQ") since my last
review, which touches several of the same files this PR owns (`RabbitmqConfig`,
`RabbitmqBaseOptions`, `RabbitmqSinkWriter`, `RabbitmqSourceReader`,
`RabbitmqSource`, plus new
`RabbitmqMessageFormat`/`RabbitmqTableConfigsValidator` files and both docs
pages), so I did a proper diff between the pre-merge tip (`08aee2b140`, my last
"Ready to merge" head from Sept 10) and this merge commit rather than treating
it as a no-op:
- `RabbitmqClient.java` — the file carrying this PR's actual fix (TLS
hardening via `configureSsl()`, `useSslProtocol(SSLContext.getDefault())` +
`enableHostnameVerification()`, and the fail-fast `queueDeclarePassive` error
handling) — has **zero diff** between the two commits. Confirmed untouched,
nothing dropped.
- `RabbitmqConfig.java` — I re-checked directly on the merge-commit head:
`serialVersionUID = -6715216959598971323L`, the `ssl` field, and the
`hasUrl`/`hasUri` mutual-exclusion check (`ILLEGAL_CONFIG` on both-present) are
all still present and unchanged. The merge only *adds* three new fields
(`format`, `protobufSchema`, `protobufMessageName`) from `#12244`, cleanly
appended after this PR's `passive` field — no overlap, no reordering that would
affect the `Serializable` layout in a way that matters (the explicit UID this
PR added is exactly what protects against that).
- `RabbitmqSinkWriter.java` — spot-checked the merge result: `#12244`'s
protobuf serializer is wired in behind a `format` switch that defaults to
`JSON` when unset, so this PR's plain JSON path (and therefore every existing
job) is unaffected; nothing from this PR's writer logic was altered.
- Docs merge (`docs/{en,zh}/connectors/{sink,source}/Rabbitmq.md`,
`incompatible-changes.md`) similarly just interleaves `#12244`'s new protobuf
option rows with this PR's `ssl`/`passive`/`uri` rows and TLS-compatibility
note — both sets of content are present on the merged head.
No source-level blockers remain open from my side; both blockers from my
Sept 1 review (missing `serialVersionUID`, undocumented TLS compatibility
change) were closed on `08aee2b140` and I've now re-confirmed they survived
this merge intact.
**CI status**: `Build` is currently **failing** on this head (`04e29b25f4`),
so — same as I just found on a sibling PR going through the identical dev-sync
— I checked the actual failing jobs rather than reporting "still red" at face
value:
- `unit-test (8, windows-latest)`: fails in
`org.apache.seatunnel.edge.agent.connector.file.FileCollectReaderBehaviorTest.rediscoversFileAfterInactiveCursorClosed`
— unrelated `seatunnel-edge-agent` module, not touched by this diff.
- `unit-test (11, ubuntu-latest)`: fails in
`org.apache.seatunnel.engine.server.TaskExecutionServiceTest.testStaleTaskDoneCleansOnlyOwnedGenerationResources`
(a Mockito `WantedButNotInvoked` on `scheduledFuture.cancel(false)`) — this is
in `seatunnel-engine-server`, a module this RabbitMQ-only PR never touches. I
don't have enough signal here to call this "known-flaky" the way I can for the
Windows edge-agent test, but the module boundary alone makes it clear it isn't
caused by anything in this diff.
Neither failure has anything to do with the RabbitMQ connector, and I
wouldn't recommend touching connector code to address them. From a
source-review perspective this remains **ready to merge**; the path forward is
a job-level rerun rather than any change to this branch. As before, I only have
comment-only access here, so a write-capable maintainer still needs to give the
binding approval and merge once CI is green.
--
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]
