[Improve][Connector-V2][Easysearch] Declarative optionRule value constraints for source and sink

[TianHengZhuang]

Purpose of the pull request

#11007 - migrate connector-easysearch source and sink option validation to declarative OptionRule value constraints.

Change log

Source:

• hosts required and not empty
• index required and not blank
• scroll_size > 0 when set
• Keep exclusive source / schema

Sink:

• hosts required and not empty
• index required and not blank
• max_batch_size > 0 when set
• max_retry_count >= 0 when set

Check list

• Tests added in EasysearchFactoryTest for required/optional bounds and exclusive source/schema
• Local mvn not run on this machine (no JDK); CI after workflow approval
• Related to [Umbrella][Improve] Migrate Connector/Transform Validation to Declarative OptionRule — Claim & Tracking #11007

[TianHengZhuang]

claimed connector-easysearch on #11007

open PR adds declarative option rules for source + sink (hosts/index required, scroll_size/batch/retry bounds) plus factory tests

[DanielLeens]

Hi @TianHengZhuang, thank you for continuing the declarative OptionRule migration (#11007) with the Easysearch connector, and welcome back. I reviewed the current head (fe9d1ef953df83139cd7c8da296e74585bfe1411) in full, including the framework code it relies on (OptionRule.Builder, Conditions, ConfigValidator, ConditionEvaluators) and the fork CI logs. The change is small, correct and well tested. Your PR description notes that you could not run Maven locally: the fork CI answers that, and the module compiles and its tests pass (details in the CI note below). I only have a few non-blocking suggestions.

What Problem Does This PR Solve?

• User pain point: EasysearchSourceFactory and EasysearchSinkFactory only declared which options exist. An empty hosts list, a blank index, scroll_size = 0, max_batch_size = 0 or a negative max_retry_count all passed ConfigValidator and only surfaced later (or misbehaved) at runtime.
• Fix approach: Attach Conditions value constraints to the option rule so ConfigValidator rejects bad values up front with an itemized OptionValidationException. Add unit tests in EasysearchFactoryTest.
• One-sentence summary: Declare hosts not empty, index not blank, scroll_size > 0 (source), max_batch_size > 0 and max_retry_count >= 0 (sink) on the Easysearch option rules.

Example for a sink with max_retry_count = -1:

before: passes ConfigValidator, job starts
after : Option validation failed (1 error): [1] ... max_retry_count ... must be greater than or equal to 0

1. Code Change Review

1.1 Core Logic Analysis

Only three files change: EasysearchSourceFactory.java, EasysearchSinkFactory.java and EasysearchFactoryTest.java (about 143 added lines, 7 removed).

Before (source, base commit): .required(HOSTS, INDEX) and a flat .optional(..., SCROLL_SIZE, ...).

After (EasysearchSourceFactory.java:62-77):

.required(HOSTS, Conditions.notEmpty(HOSTS))
.required(INDEX, Conditions.notBlank(INDEX))
.optional(SCROLL_SIZE, Conditions.greaterThan(SCROLL_SIZE, 0))
.optional(USERNAME, PASSWORD, SCROLL_TIME, QUERY, TLS_...)   // SCROLL_SIZE moved out of this list
.exclusive(SOURCE, ConnectorCommonOptions.SCHEMA)               // unchanged

The sink (EasysearchSinkFactory.java:37-62) does the same for HOSTS, INDEX, MAX_BATCH_SIZE (> 0) and MAX_RETRY_COUNT (>= 0).

Key findings:

• Compiles against the real framework API. I checked each call against OptionRule.Builder at current dev: required(Option, Condition, Condition...) and optional(Option, Condition, Condition...) exist, and the literal 0 matches Option<Integer> for SCROLL_SIZE, MAX_BATCH_SIZE and MAX_RETRY_COUNT. notEmpty on Option<List<String>> and notBlank on Option<String> are type-correct.
• Nothing was dropped. The moved options were removed from the old flat .optional(...) lists, so there is no duplicate-option failure in verifyOptionOptionsDuplicate, and the set of declared keys is identical, so ConfigValidator.validateUnknownKeys behaves as before. exclusive(SOURCE, SCHEMA) is untouched.
• Optional constraints do not fire on defaults. ConfigValidator.isConstraintApplicable only evaluates a constraint on an optional option when the user actually set it. The defaults (scroll_size = 100, max_batch_size = 10, max_retry_count = 3) all satisfy the new bounds anyway, so a job that omits these options is unaffected.
• No double reporting for missing required options. When hosts or index is missing, collectErrors records the "required" error and skips the value constraint via structurallyAbsentKeys, so the user gets one clear error, not two.
• Runtime coverage. Both rules are reached on the normal user path: FactoryUtil.createAndPrepareSource and FactoryUtil.createAndPrepareSink call ConfigValidator.of(context.getOptions()).validate(factory.optionRule()) (FactoryUtil.java:191, :203, :252, :279). Validation therefore happens before EasysearchClient.createInstance, EasysearchSourceSplitEnumerator or EasysearchSinkWriter are built.

Runtime path (validation only, no data path, no checkpoint or serialization involved):

job config -> FactoryUtil.createAndPrepare{Source,Sink}
  -> ConfigValidator.validate(factory.optionRule())
       -> required(hosts, index) present?  -> value constraints (notEmpty / notBlank)
       -> optional constraints only for options the user set (scroll_size, max_batch_size, max_retry_count)
       -> exclusive(source, schema)
  -> OptionValidationException with all violations, or continue to createSource / createSink

1.2 Compatibility Impact

Partially incompatible (very small blast radius). Option names, defaults, serialized state and checkpoint format are unchanged, and the in-repo Easysearch E2E configs (easysearch_source_and_sink.conf, easysearch_source_and_sink_with_save_mode.conf) use a non-empty hosts, a non-blank index and none of the newly bounded options, so they are not affected.

Values that passed validation before and are rejected now:

• hosts = [] and a blank index: neither can produce a working job. I did not trace the third-party client's handling of an empty host array, so that part is an inference, not an observation.
• max_batch_size = 0: this one did run before. EasysearchSinkWriter.java:65-84 builds new ArrayList<>(0), and write flushes when requestEzsList.size() >= maxBatchSize, so 0 behaved like "flush after every row". It is now rejected. A negative value already failed earlier (new ArrayList<>(-1) throws IllegalArgumentException).
• scroll_size = 0: whether the server accepted size = 0 in a scroll request is server behavior that I could not verify from the source, so I make no claim about it.

This is a legitimate tightening of degenerate values, so I do not consider it a blocker (see Issue 1 for a documentation suggestion).

1.3 Performance / Side-Effect Analysis

Negligible. The constraints run once per source or sink creation inside ConfigValidator, not on the data path. No threads, locks, I/O, retries or logging are added.

1.4 Error Handling and Logging

ConfigValidator aggregates all violations into one OptionValidationException, nothing is swallowed, and only option keys and constraint descriptions are reported (no values such as username or password are logged). No blocking issues. Suggestions:

Issue 1: The newly rejected values are not written down for users

• Location: docs/en/connectors/sink/Easysearch.md:57-58, docs/en/connectors/source/Easysearch.md:64 (and the zh counterparts), docs/en/introduction/concepts/incompatible-changes.md
• Problem: the option tables and the scroll_size / max_batch_size / max_retry_count sections do not mention the accepted ranges, and max_batch_size = 0 (which used to run, see 1.2) is not recorded as a behavior change.
• Potential risk: users only learn the limits from the validation error; a user who set max_batch_size = 0 on purpose gets a new failure after upgrading with no upgrade note.
• Best improvement: add a short range note to those rows in docs/en and docs/zh (for example "must be greater than 0" / "must be greater than or equal to 0"), as the merged Druid and Firestore migrations did (#12250, #12278). Optionally add a one-line "Behavior change" entry under ## dev in incompatible-changes.md for max_batch_size = 0, or, if you prefer to preserve today's behavior exactly, use Conditions.greaterOrEqual(MAX_BATCH_SIZE, 0). Either is fine; I lean toward keeping > 0 and adding the note.
• Severity: Low
• Raised by another reviewer: No

Issue 2: Tests assert only the exception type, not which option was rejected

• Location: EasysearchFactoryTest.java (the assertThrows(OptionValidationException.class, ...) calls, for example the nonPositiveScrollSizeIsRejected, nonPositiveMaxBatchSizeIsRejected and negativeMaxRetryCountIsRejected cases)
• Problem: each negative case mutates one value of an otherwise valid config, so today the exception can only come from that value (the valid-config tests prove the baseline). But a future refactor that makes the config invalid for a different reason would keep these tests green. Boundary-passing cases exist only for scroll_size = 1, max_batch_size = 1, max_retry_count = 0; a negative scroll_size / max_batch_size is not asserted.
• Potential risk: low, only weaker regression detection later.
• Best improvement: keep the exception in a variable and assert its message contains the offending key (for example scroll_size), and optionally add one -1 case for scroll_size and max_batch_size.
• Severity: Low
• Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Imports are explicit (no wildcards), Conditions is imported normally, formatting follows Spotless (the fork CI ran spotless-check on connector-easysearch and the Code style job passed), no System.out, no new classes so no license-header or class-Javadoc concern. The constraint declarations read like a specification of the option contract, which is exactly the goal of the umbrella issue.

2.2 Test Coverage and Test Stability

EasysearchFactoryTest now has 11 tests covering: valid source and sink configs, valid boundary values, blank and empty index, empty hosts, missing hosts / index, scroll_size = 0, max_batch_size = 0, max_retry_count = -1, and the source + schema exclusivity. I checked the input types of every test map against the option definitions (hosts and source as lists, schema as a map, numeric values as ints), so unlike the wrong-typed value that made a test in #12404 fail, nothing here can raise IllegalArgumentException instead of OptionValidationException. The CI log from the fork confirms it: EasysearchFactoryTest ran 11 tests, 0 failures, 0 errors on the Windows job.

Stability rating: Stable. The tests are pure in-memory validator calls (EasysearchFactoryTest.java:37-149): no sleeps, ports, containers, static state or external resources. No E2E change is needed because only config validation changes and the existing E2E configs are unaffected.

2.3 Documentation Updates

Not updated (Issue 1, Low). This matches several merged declarative migrations that did not touch docs, so I am not treating it as a blocker.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix in the right place and long-term direction: the declarative Conditions mechanism is the established way to express these checks, and no imperative if/throw is duplicated in the connector.

3.2 Maintainability

Good. Constraints sit next to the option declarations in the two factories; the split of .required / .optional mirrors the umbrella issue's migration guide (section A, numeric range).

3.3 Extensibility

Adding further constraints (for example an upper bound for scroll_size) is a one-line change per option.

3.4 Historical-Version Compatibility

No serialization, state, checkpoint or savepoint format is touched, and no option is renamed or removed. The only historical difference is the small set of newly rejected degenerate values listed in 1.2. I did not run an upgrade test; this is a source-level conclusion for a validation-only change.

4. Issue Summary

No blocking issues found.

#	Issue	Location	Severity
1	Newly rejected values (notably max_batch_size = 0) and ranges are not documented	docs/en and docs/zh Easysearch source/sink pages, incompatible-changes.md	Low
2	Negative tests assert only the exception type, not the offending option	EasysearchFactoryTest.java	Low

Remaining risk: CI is currently red on one job that is unrelated to this change (next section).

CI status (head fe9d1ef953d): the apache-side Build check is failure, which points to the fork run (TianHengZhuang/seatunnel, Build run 35554994205). In that run, 25 jobs succeeded (including Code style, License header, unit-test (8, ubuntu-latest), unit-test (11, ubuntu-latest), unit-test (11, windows-latest) and all eight updated-modules-integration-test parts on both JDKs), and exactly one job failed: unit-test (8, windows-latest). The failing test is TaskExecutionServiceTest.testStaleTaskDoneCleansOnlyOwnedGenerationResources in seatunnel-engine-server (Mockito.verify(oldTimerFlushFuture).cancel(false), "zero interactions with this mock", TaskExecutionServiceTest.java:639). This PR changes no engine file, the test came in with #12238 which is already in this branch's base, and neither the test nor TaskExecutionService changed on dev since, so this failure is not caused by this PR and syncing dev will not fix it. I saw it in this single run, so I cannot say from this evidence alone whether it is deterministic or flaky on Windows. In the same run, the connector-easysearch module compiled, passed spotless-check, and its tests passed (EasysearchFactoryTest: 11 run, 0 failures). Suggested action: re-run only the failed job (unit-test (8, windows-latest)) in your fork's Actions tab; if it fails again on the same test, a committer can decide whether it needs its own fix, independent of this PR. The downstream integration jobs that show as skipped are skipped by the workflow, not by this change.

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers - must be fixed: none from the code side. The only open item is the unrelated red Windows job above.
2. Recommended fixes - non-blocking: Issue 1 (document the ranges and the max_batch_size = 0 behavior change) and Issue 2 (assert the offending option in the negative tests).

Overall assessment: a focused, correct migration with good tests, and it follows the framework's intended usage. Compared with the sibling Fake connector PR (#12404, where I requested changes earlier), the same classes of problem were checked here: the test inputs are correctly typed for their options (that was the High blocker there), no unrelated behavior is silently tightened beyond degenerate values, and Spotless is clean. The remaining overlap is only the documentation note, which is Low here because the affected values are degenerate, whereas the Fake PR rejected values such as row.num = 0 and split.num = 0 that run today. No existing reviewer has approved or requested changes on this PR yet. Thanks again for the contribution, and great work keeping the change small and focused.

[nzw921rx]

please rerun failed job.

[DanielLeens]

Thanks for the nudge. I just re-checked live: the head commit is still fe9d1ef953d (unchanged) and the Build check on that commit is still failure, completed at 2026-09-21T04:18:47Z - I don't see a newer run yet, so the rerun hasn't landed on this commit.

As noted in my review, the only failing job in that run is unit-test (8, windows-latest), and the failure is TaskExecutionServiceTest.testStaleTaskDoneCleansOnlyOwnedGenerationResources in seatunnel-engine-server. This PR does not touch any engine code, and that test/class hasn't changed on dev since, so it isn't caused by this diff - syncing dev alone will not fix it either.

Re-running that job has to happen from the fork's own Actions tab (GitHub only lets users with write access to TianHengZhuang/seatunnel trigger a rerun there), so @TianHengZhuang, could you click "Re-run failed jobs" on that run? Once it's green (or if it fails again on the same test, which would help confirm this is flaky rather than PR-related), I'll take another look. To be clear, this doesn't change my assessment of the code itself, which I've already approved.