Skip to content

[Improve][Connector-V2] Migrate MQTT connector validation to declarative OptionRule#11461

Open
ealeonraz wants to merge 1 commit into
apache:devfrom
ealeonraz:improve-mqtt-declarative-validation
Open

[Improve][Connector-V2] Migrate MQTT connector validation to declarative OptionRule#11461
ealeonraz wants to merge 1 commit into
apache:devfrom
ealeonraz:improve-mqtt-declarative-validation

Conversation

@ealeonraz

Copy link
Copy Markdown

Purpose of this pull request

Part of #11007 (connector-mqtt slice). Migrates the MQTT connector's option validation from imperative if/throw checks to declarative optionRule() + Conditions.*, so invalid configs are caught at --check/submission time and constraints are exposed via option-rule metadata.

Migrated rules:

  • Source (MqttSourceFactory): qos must be 0 or 1, format must be json/text (case-insensitive, via a shared MqttFormatValidator ConditionExtension), reconnect_timeout > 0, max_queue_size > 0, and client_id required and non-blank when clean_session=false. The client_id rule is expressed as a conditional requirement plus a conditional notBlank value constraint, because conditional value constraints alone are skipped when the option is absent — a single rule would silently accept clean_session=false without a client_id.
  • Sink (MqttSinkFactory): qos must be 0 or 1, format must be json/text, batch_size >= 1.

Kept in runtime code (per the migration guide's "not suitable for declarative rules" section): broker connection error handling and the serialization-format dispatch default branch. No options were renamed and no defaults changed.

Does this PR introduce any user-facing change?

No config option changes. Invalid option values are now rejected during config validation (OptionValidationException at --check/submission time) instead of IllegalArgumentException at connector initialization.

How was this patch tested?

Existing imperative-validation unit tests were migrated to ConfigValidator-based tests against the factory option rules, plus new boundary cases (qos 0/1 accepted, batch_size 1 accepted, case-insensitive formats, and all three client_id scenarios: absent fails, blank fails, present passes).

./mvnw -pl seatunnel-connectors-v2/connector-mqtt test
Tests run: 42, Failures: 0, Errors: 0, Skipped: 0

./mvnw spotless:apply and ./mvnw -DskipTests verify run clean on the module.

Check list

  • No new Jar binary package added
  • No documentation update needed (option semantics unchanged; only the enforcement point moved)
  • No incompatible change (incompatible-changes.md not affected)
  • Not a new connector (plugin-mapping.properties / seatunnel-dist / labeler / e2e / plugin_config unchanged)

@nzw921rx

Copy link
Copy Markdown
Collaborator

@ealeonraz Thank you for your contribution! This improvement further enhances the configuration verification system of SeaTunnel, allowing configuration issues to be exposed earlier and more clearly, which is very helpful in improving overall usability and maintainability.🚀

pls enable CI: https://github.com/apache/seatunnel/pull/11461/checks?check_run_id=87482860873

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I rechecked the latest head cee75286b7a6 against the normal MQTT source/sink creation path, and I do not see a source-level blocker on the current diff.

The validation move looks consistent to me:

  • source path: MqttSourceFactory.optionRule() -> MqttSource(ReadonlyConfig) -> MqttSourceConfig
  • sink path: MqttSinkFactory.optionRule() -> MqttSink.createWriter(...) -> MqttSinkWriter
  • the new factory tests cover the moved constraints (qos, format, reconnect_timeout, max_queue_size, batch_size, and the persistent-session client_id requirement), so this is not just deleting checks without replacement

The remaining gate is CI rather than source logic. Right now Build is ACTION_REQUIRED because workflow run detection failed, and the check summary is explicitly asking for two things:

  1. enable GitHub Actions in the fork
  2. trigger a fresh run

This branch is also behind the latest dev by 1 commit (ahead_by=1, behind_by=1, compare status diverged), so the lowest-cost next step is:

  • sync with the latest dev
  • enable Actions in the fork if they are still disabled there
  • push an empty commit or otherwise retrigger CI

If the refreshed run still fails after that, please share the new check link here and I can help narrow it down further.

@ealeonraz
ealeonraz force-pushed the improve-mqtt-declarative-validation branch from 7f0f6ac to 8175004 Compare July 16, 2026 17:15
@ealeonraz

Copy link
Copy Markdown
Author

@DanielLeens Done on both counts: Actions enabled in the fork, and the branch is rebased onto the latest dev (3656ba8) — the PR is now a single commit.

Refreshed run: https://github.com/ealeonraz/seatunnel/actions/runs/29519036639

Everything is green except the unit-test job, which fails in a test unrelated to this PR: MetricsApiTest.metricsApiTest in seatunnel-engine-server (metrics endpoint returns 500; the log shows HazelcastInstanceNotActiveException: State: SHUT_DOWN surfacing through ExceptionHandlingFilter — the request hits a stale instance on the hard-coded port 8080).

Evidence it's independent of this change:

  • seatunnel-engine-server test scope doesn't include connector-mqtt (only connector-fake/console/file-local), so this diff isn't on that test's classpath
  • The upstream nightly Schedule Backend on clean dev at the same commit fails the identical test: https://github.com/apache/seatunnel/actions/runs/29515660951 (unit-test (11, ubuntu-latest))
  • It failed consistently across JDK 8/11 ubuntu cells in three runs here (Windows cells skip it via @DisabledOnOs), including a failed-jobs rerun

I didn't touch the engine test in this PR to keep the scope to the connector-mqtt slice, but I'm happy to file a separate issue for MetricsApiTest (and take a crack at a fix) if that's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants