DanielLeens commented on PR #12048:
URL: https://github.com/apache/seatunnel/pull/12048#issuecomment-5881949048
Thanks for the careful follow-up, @SEZ9 — happy to close the loop on
F1/F2/F3/F4/F6/F7/F8 with direct pointers rather than descriptions, and to
correct one thing in my own last review while I'm at it.
**First, a head update.** Since my `56794bc19d3e` review, one more commit
landed: `49c2f9059a7e` ("Merge remote-tracking branch 'upstream/dev' into
feature/GH-10753-azure-event-hubs-source"). I diffed
`56794bc19d3e..49c2f9059a7e` and it pulls in 32 commits from `dev` but touches
zero files under `connector-azure-event-hubs/` or
`connector-azure-event-hubs-e2e/` — the only PR-adjacent file it touches at all
is `tools/dependencies/known-dependencies.txt`, and that hunk is just a
trailing blank-line addition unrelated to this PR's own entries. So everything
below is still verified against the same connector code you and I have both
been looking at; nothing shifted under us.
**CI correction (my own last review is now stale here).** My `56794bc19d3e`
review said CI was red with 10 unrelated failures (Windows `PayPalClientTest`,
`CheckpointCoordinatorFailoverIT`, S3/Databend/Paimon flakes). At the current
head `49c2f9059a7e`, the `Build` check is now green (passed in ~8.5h). Since
this was a pure `dev`-sync with no code change on this PR's side, that's
exactly what I'd expect — the prior red run was `dev`-side noise, not something
this PR introduced, and it's now cleared itself out. Conclusion stands, and CI
is no longer a reason to hold this.
Now, per item:
**F1 (retention-trimmed checkpoint recovery, HIGH).** Confirmed with the
actual paragraph, not just my earlier summary:
`docs/en/connectors/source/AzureEventHubs.md:114` reads:
> "To recover from a trimmed checkpoint position, stop the failing job and
start a new job without restoring the old source state. Choose `start_mode`
explicitly: `earliest` replays all still-retained events, including events
already processed, while `latest` skips the existing backlog. ... Restarting
with the same checkpoint or changing only `start_mode` does not reset the saved
position."
That's an explicit "without restoring the old checkpoint state" instruction
— it directly answers what you asked for. I'd call F1 resolved.
**F2 (`ServicesResourceTransformer` inheritance / packaged jar contents).**
I want to be precise about what I can and can't confirm here in a reply-only,
no-build pass: I can (and did, in my last review) confirm statically that root
`pom.xml`'s `default-shade` execution declares `ServicesResourceTransformer`
and `connector-azure-event-hubs/pom.xml` declares the same shade goal with no
`<id>` override and no `<transformers>` of its own, so Maven merges the
parent's transformer in untouched — there's no config path by which the
relocated service-provider files would be dropped. What I *can't* do from here
is literally list the entries inside a built jar, since that needs an actual
`package` run, which is outside this reply-only re-verification (no local build
in this pass). This item is already scored Low/non-blocking in my review
precisely because it's a build-config correctness question, not a
runtime-behavior one — if you want jar-content certainty before closing it,
that's r
easonably asked of the author (`jar tf` on the built artifact) or left to CI's
own packaging step, which would fail the module if the shade config were
actually broken.
**F3 (no SAS key on any output path).** Unchanged, re-confirmed at current
head: `AzureEventHubsSourceConfigTest.java`, method
`entityPathRejectionAndStringRepresentationsDoNotExposeCredentials` (lines
41-66), asserts the full `CONNECTION_STRING` string is absent from
`config.toString()`, `AzureEventHubsSourceSplit#toString()`,
`exception.getMessage()`, and `ExceptionUtils.getMessage(exception)`
(cause-chain walk) — across two different hub values.
**F6 (prefetch_count >= max_batch_size enforced at the factory, not just the
reader).** Unchanged, re-confirmed: `AzureEventHubsSourceFactory.optionRule()`
(`AzureEventHubsSourceFactory.java:47-49`) builds its rule from the *same*
`AzureEventHubsSourceConfig.optionRuleBuilder()`
(`AzureEventHubsSourceConfig.java:84-110`) whose `PREFETCH_COUNT` rule includes
`Conditions.greaterOrEqualField(PREFETCH_COUNT, MAX_BATCH_SIZE)` (`:106-109`).
Test `factoryRulesRejectBlankAndCrossFieldOptions`
(`AzureEventHubsSourceConfigTest.java:192-206`) exercises this through
`ConfigValidator.of(...).validate(new
AzureEventHubsSourceFactory().optionRule())` — the factory's own rule object,
not just the reader-side `from()` gate.
**F4 (tracking reference for shaded Netty/Reactor/Proton-J versions).** Fair
ask, and to be direct: no follow-up ticket exists for this yet as far as I can
find — I checked the PR description and the doc's "Retry And Failure Behavior"
section and neither links one. This stays Low/non-blocking either way (it's a
disclosed, isolated-by-relocation risk, not a functional bug), but I agree a
tracked issue would be better than a one-time disclosure. @goutamadwant, would
you be open to filing a short follow-up issue for this so it's traceable
outside this PR thread?
**F7 (behavior when live partition count exceeds the checkpointed set).**
Restating cleanly: it's neither fail-fast nor log-and-continue — it's silent
non-discovery. `AzureEventHubsSourceSplitEnumerator.run()` (lines 67-75) only
calls `discoverSplits()` when `restored == false` (line 69); on a restored
enumerator, that branch never executes at all. The only log statement in this
class (`log.info("Discovered {} Event Hubs partition(s)...")`, lines 94-98)
lives inside `discoverSplits()`, so it never fires on restore either — there's
no informational log telling the operator new partitions exist. New partitions
are simply invisible to a restored job until it's restarted without prior
state, which is exactly what `docs/en/connectors/source/AzureEventHubs.md:106`
documents ("Partitions added after the job starts are not picked up dynamically
or when restoring existing source state").
**F8 (connector-level reconnect/retry tuning / where the SDK-default retry
window is documented).** No connector-level retry tuning was added — confirmed
both in code (no retry/backoff option in
`AzureEventHubsSourceOptions`/`AzureEventHubsSourceConfig`) and in docs:
`docs/en/connectors/source/AzureEventHubs.md:118-120`, under "Retry And Failure
Behavior": "The Azure SDK applies its built-in AMQP retry policy. Outages that
outlast that retry budget surface as source task failures. ... This connector
version does not expose Azure SDK retry or backoff settings." SeaTunnel's own
checkpoint/job recovery is the layer that handles restart after that, which
matches the runtime trace in my last review.
With F1/F3/F6/F7/F8 all confirmed against source/docs and F2/F4 left as
explicitly-scoped non-blocking follow-ups (one a build-verification step, the
other a "please file a tracking issue" ask), I don't see anything here that
changes my conclusion: **Ready to merge**, and CI is green on the current head.
Thanks again for pushing for line-level confirmation instead of taking the
summaries at face value — that's exactly the right instinct on a first-time
connector this size.
--
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]
