DanielLeens commented on PR #11985: URL: https://github.com/apache/seatunnel/pull/11985#issuecomment-5476959778
Thanks for circling back with specifics, @SEZ9 — I re-checked each of the four points against the current head (`29a172c05`) rather than just my prior notes, since two of them turned out to need a correction on my side. 1. **Removed builder-level validation** — I traced every current call site of `PulsarConsumerConfig.builder()` / `PulsarAdminConfig.builder()` / `PulsarClientConfig.builder()`: they only exist inside `PulsarSource` (lines 245, 254, 262) and `PulsarSink` (line 64), and both classes are only ever constructed by `PulsarSourceFactory#createSource()` / `PulsarSinkFactory#createSink()`, which `FactoryUtil` invokes strictly after `ConfigValidator.validate(factory.optionRule())` has already passed. So today there is no call path in this connector that reaches the builders without first going through the declarative checks — the removal doesn't currently open an unguarded route. I agree a defensive builder-level check has some value if a future caller ever constructs these builders directly outside the factory path, but given no such caller exists right now, I don't think it's a blocker. 2. **Optional `SUBSCRIPTION_NAME` unguarded for single-table jobs** — this one I don't think holds up: `PulsarSourceFactory#createSource()` calls `validateSourceOptions(context.getOptions())` (`PulsarSourceFactory.java:100`) on every job-submission path, and that method (`:112-121`) explicitly throws if `TABLE_CONFIGS` is absent *and* `SUBSCRIPTION_NAME` is absent — so a single-table job that omits the option entirely fails fast with "Single-table Pulsar source must configure 'subscription.name'." before `PulsarSource` is even built. For the multi-table path, `PulsarMultiTableConfig.validateTableConfig()` (`:284-288`) independently throws on `isBlank(subscriptionName)` per table. Between the two, both "omitted entirely" (single-table) and "blank" (either mode) are covered on the real path — could you point me to a specific config shape you think still slips through? Happy to dig further if so. 3. **Inconsistent `notBlank` on `topic`/`topic-pattern`** — you're right, and I should have caught this in my own re-review. Checked `PulsarSourceFactory.optionRule()` and `PulsarSinkFactory.optionRule()` on the current head: `CLIENT_SERVICE_URL`, `ADMIN_SERVICE_URL`, and `SUBSCRIPTION_NAME` all got `Conditions.notBlank(...)` in this PR, but `TOPIC`/`TOPIC_PATTERN` are still declared via plain `.optional(...)`/`.exclusive(...)` with no blank guard. It's a pre-existing gap (not introduced by this PR — `topic` was never blank-guarded before this diff either), so I don't think it should block this PR specifically, but it directly undercuts the PR's own "validation should be complete and declarative" rationale. Worth a quick fast-follow to apply the same `Conditions.notBlank(...)` treatment there. 4. **Test robustness** — also fair. `testMissingClientServiceUrlFails`/`testBlankClientServiceUrlFails` (and the admin/subscription equivalents) both only assert `exception.getMessage().contains(<option key>)`, which is true for either a "required" failure or a "notBlank" failure — the tests don't actually distinguish which validator tripped. Same pattern separately at lines 79/88/137 for the "mutually exclusive"/"bundled" checks. None of this makes the tests wrong (they do correctly assert failure for each blank input, and CI is green), but tightening the assertions to check the specific violated-condition text, plus adding the positive topic-pattern-only case and a blank per-table `subscription.name` case, would make regressions easier to catch later. Non-blocking. Net: items 1 and 2 check out as already covered on every real path, so I'm not changing my Approved recommendation. Items 3 and 4 are legitimate, scoped, non-blocking follow-ups — thanks for pushing on this, the topic/topic-pattern gap in particular is worth its own quick PR. -- 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]
