SEZ9 commented on PR #11985:
URL: https://github.com/apache/seatunnel/pull/11985#issuecomment-5476208794
Thanks @davidzollo for the review and the +1!
Before we merge, I'd like to circle back on a few points from my earlier
review that I don't believe have been addressed yet:
1. **Removed builder-level validation** — Dropping the blank-subscription
check in `PulsarConsumerConfig.Builder` and the URL checks in
`PulsarAdminConfig`/`PulsarClientConfig` means the declarative `notBlank` rules
only protect factory-created pipelines. Direct builder callers and per-table
`subscription.name` values are now unguarded. Could we either keep the builder
checks as a defensive layer or confirm all call paths go through the factory
validation?
2. **Optional `SUBSCRIPTION_NAME`** — With the builder check removed and the
option still optional in the OptionRule (`PulsarSourceFactory`), a single-table
job that omits the subscription entirely has no guard. Please add coverage or
tighten the rule for that case.
3. **Inconsistent `notBlank` application** — Sink `topic` and source
`topic`/`topic-pattern` still accept blank strings, which contradicts the PR's
own rationale. Can we apply the same treatment there?
4. **Test robustness** — Several new assertions in `PulsarSinkFactoryTest`
and `PulsarSourceFactoryTest` couple to framework-owned message fragments
("bundled", "mutually exclusive", "exactly one option must be set") or only
check that the option key appears in the message, which can't distinguish a
`notBlank` violation from an unrelated failure. Asserting on more
specific/stable signals would help. Also, a positive topic-pattern-only test
and a test for the blank per-table `subscription.name` scenario are still
missing.
None of these are blockers on their own, but I'd like at least items 1 and 2
resolved (or explicitly justified) before merging, since they affect runtime
robustness rather than just tests. Happy to discuss if there's context I'm
missing!
<!-- streview-comment:696 -->
--
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]
