[Improve][Connector-V2][HBase] Migrate timestamp validation to OptionRule

[goutamadwant]

Purpose of this pull request

Part of #11007.

HBase source timestamp constraints were validated only when HbaseClient created the scan. This PR adds the same constraints to HbaseSourceFactory#optionRule() so invalid connector configuration is rejected during factory validation.

The change:

• requires start_timestamp to be non-negative and an explicitly configured end_timestamp to be positive
• requires start_timestamp to be less than end_timestamp when both are configured
• preserves the existing runtime checks for direct or programmatic construction of HbaseClient
• adds focused factory validation coverage for optional, valid, negative, equal and reversed timestamp ranges

Does this PR introduce any user-facing change?

Yes.

Invalid HBase timestamp configurations are now rejected during connector option validation instead of failing later while the HBase scan is created. Valid configurations keep the existing behavior.

The English and Chinese HBase documentation now distinguish the non-negative start bound from the positive end bound and retain the start_timestamp < end_timestamp constraint.

How was this patch tested?

Added factory validation tests covering:

• neither timestamp configured
• only start_timestamp configured
• only end_timestamp configured
• a valid timestamp range
• negative start and end timestamps
• a zero end timestamp
• the minimum valid [0, 1) range
• equal start and end timestamps
• a start timestamp greater than the end timestamp

Focused verification:

./mvnw -pl seatunnel-connectors-v2/connector-hbase -Dtest=HbaseFactoryTest test

The full connector-hbase package was also run locally. All 43 tests passed.

Check list

• If any new Jar binary package adding in your PR, please add License Notice according
  New License Guide
• If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
• If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.
• If you are contributing the connector code, please check that the following files are updated:
  1. Update plugin-mapping.properties and add new connector information in it
  2. Update the pom file of seatunnel-dist
  3. Add ci label in label-scope-conf
  4. Add e2e testcase in seatunnel-e2e
  5. Update connector plugin_config

No checklist item applies to this change. It does not add a binary, dependency, connector, packaging entry or incompatible behavior. The existing English and Chinese HBase documentation was updated to match the validated bounds.

[DanielLeens]

Thanks for this one, @goutamadwant — this is a clean, well-scoped contribution to the #11007 umbrella, and I appreciate that you kept the runtime guard in HbaseClient instead of deleting it. I traced the full validation chain end to end and the change is correct and backward compatible. I have no blocking findings; there is one real completeness gap and a couple of polish items below.

What Problem Does This PR Solve?

Today an invalid HBase source timestamp configuration (start_timestamp = -1, or start_timestamp >= end_timestamp) is only rejected deep inside HbaseClient.applyTimeRange() when the Scan object is assembled — i.e. after the job has been submitted, the source has been created, splits have been enumerated and a reader is already running. The user pays for a full submission round-trip to learn about a typo.

This PR declares the same constraints on HbaseSourceFactory#optionRule() using the new Conditions DSL, so ConfigValidator rejects the bad config at factory-validation time, before any HBase connection is opened.

One-sentence summary: move an existing runtime precondition forward to declarative option validation, without removing the runtime precondition.

1. Code Change Review

1.1 Core Logic Analysis

The whole production-side change is in seatunnel-connectors-v2/connector-hbase/src/main/java/org/apache/seatunnel/connectors/seatunnel/hbase/source/HbaseSourceFactory.java:57-70.

Before — the two timestamp options were plain metadata-only entries in the optional(Option...) list:

.optional(
        ...
        HbaseSourceOptions.END_ROW_INCLUSIVE,
        HbaseSourceOptions.START_TIMESTAMP,
        HbaseSourceOptions.END_TIMESTAMP)

After — they are split into a second optional(...) call that carries value constraints:

.optional(
        HbaseSourceOptions.START_TIMESTAMP,
        HbaseSourceOptions.END_TIMESTAMP,
        Conditions.greaterOrEqual(HbaseSourceOptions.START_TIMESTAMP, 0L),
        Conditions.greaterOrEqual(HbaseSourceOptions.END_TIMESTAMP, 0L),
        Conditions.lessThanField(
                HbaseSourceOptions.START_TIMESTAMP,
                HbaseSourceOptions.END_TIMESTAMP))

Correctness analysis — I walked each layer rather than trusting the DSL surface:

1. Overload resolution. OptionRule.Builder.optional(Option, Option, Condition, Condition...) exists at seatunnel-api/.../OptionRule.java:359-373. It registers both options in optionalOptions and appends the three conditions to valueConstraints. Because the two options were moved out of the first optional(...) group rather than duplicated, verifyOptionOptionsDuplicate() (OptionRule.java:548) does not fire. Good.

2. Applicability when an option is absent. This is the part that actually decides whether the change is safe for existing jobs. ConfigValidator.collectErrors() (ConfigValidator.java:230-249) calls isConstraintApplicable() (ConfigValidator.java:290-299) before evaluating each constraint. Since neither timestamp is an AbsolutelyRequiredOptions, it falls through to anyOrSegmentFullyPresent() (ConfigValidator.java:305-337), which collects getOption() and getCompareOption() per AND-segment and requires all of them to be present:

   • greaterOrEqual(START_TIMESTAMP, 0L) -> segment {start_timestamp} -> skipped when absent.
   • lessThanField(START_TIMESTAMP, END_TIMESTAMP) -> segment {start_timestamp, end_timestamp} -> skipped unless both are set.

   This is exactly what you need here, and it is why the null/null, 0/null and null/1000 cases in your test pass rather than tripping the v != null && ... short-circuit in ConditionEvaluators.java:64-75.

3. Both options are noDefaultValue() (HbaseSourceOptions.java:51-63), so config.get(option) returns null when unset and hasOption() is false. If either had carried a default, the constraint would have been evaluated against that default for every job — worth calling out because this pattern is easy to get wrong on other connectors in the #11007 migration.

4. Numeric comparison. ConditionEvaluators.compareNumberValues() (ConditionEvaluators.java:202-217) takes the Long.compare(a.longValue(), b.longValue()) branch for two Long values. No precision or boxing surprise.

Runtime path — where this actually executes:

SeaTunnel client submits job
  -> FactoryUtil.createAndPrepareSource(...)            seatunnel-api/.../FactoryUtil.java:184-194
     -> ConfigValidator.of(context.getOptions())
          .validate(factory.optionRule())               FactoryUtil.java:191   <-- NEW rejection point
        -> collectErrors -> isConstraintApplicable -> ConditionEvaluators.evaluate
     -> HbaseSourceFactory.createSource(context)
        -> HbaseParameters.buildWithSourceConfig(...)   HbaseParameters.java:168-173
  -> ... split enumeration, reader start ...
  -> HbaseClient.buildScan(split, params, columns)      HbaseClient.java:378-393
     -> HbaseClient.applyTimeRange(scan, params)        HbaseClient.java:395-416  <-- OLD rejection point (kept)

Note that FactoryUtil.restoreAndPrepareSource() (FactoryUtil.java:203) runs the same validation, so a savepoint/restore of an already-running job with a legal config is unaffected.

Does the normal user path reach the changed logic? Yes — createAndPrepareSource is the standard source construction path for every engine, so this is a main-path change, not an edge/recovery-only one. It only fires on configs that were already illegal, which is what makes it safe.

1.2 Compatibility Impact

Fully compatible. I enumerated the accept/reject matrix against the pre-existing runtime guard at HbaseClient.java:395-416:

Config	Old behavior	New behavior
neither set	accepted (early return, no time range)	accepted (both constraints skipped)
start >= 0 only	accepted (min=start, max=Long.MAX_VALUE)	accepted
end > 0 only	accepted (min=0)	accepted
start < end, both >= 0	accepted	accepted
start < 0 or end < 0	rejected at scan build	rejected at submission
start >= end, both set	rejected at scan build	rejected at submission

There is no configuration that the new rule rejects and the old code accepted, which is the property that matters for existing production jobs. The only user-visible delta is when and with which message the rejection happens. No option was renamed, removed, or given a new default, so incompatible-changes.md correctly needs no entry.

1.3 Performance / Side-Effect Analysis

Negligible and one-shot. optionRule() is built once per source construction; three extra Condition objects and at most three evaluator lookups on an EnumMap. No per-record, per-split or per-checkpoint cost. No new threads, buffers, connections or class loading. Nothing to flag.

1.4 Error Handling and Logging

Issue 1

• Location: seatunnel-connectors-v2/connector-hbase/src/main/java/org/apache/seatunnel/connectors/seatunnel/hbase/source/HbaseSourceFactory.java:60-70
• Problem description: The declarative rule is not a full parity migration of the runtime guard. HbaseClient.applyTimeRange() (HbaseClient.java:410-414) substitutes min = 0 when start_timestamp is absent and then rejects min >= max. So end_timestamp = 0 with start_timestamp unset is illegal at runtime (0 >= 0), but the OptionRule accepts it: greaterOrEqual(END_TIMESTAMP, 0L) passes, and lessThanField(...) is skipped because start_timestamp is absent (ConfigValidator.java:305-337). The same holds for end_timestamp = 0 in general.
• Potential risk: Low in practice — the runtime guard still catches it, so this is a completeness gap rather than a regression. But the PR description states the goal as "invalid connector configuration is rejected during factory validation", and one class of invalid config still slips through to the scan-build stage. It also disagrees with docs/en/connectors/source/Hbase.md:117, which says end_timestamp "must be >= 0" and would therefore lead a user to believe end_timestamp = 0 alone is valid.
• Best improvement:
  • Option A (declarative, preferred): tighten the standalone bound on the end side, since the effective lower bound is 0 when start_timestamp is unset:

    Conditions.greaterThan(HbaseSourceOptions.END_TIMESTAMP, 0L)

    This is exactly equivalent to the runtime semantics (end_timestamp = 0 is always empty, whether or not start_timestamp is set, because start_timestamp >= 0 is enforced), and it closes the gap with a one-word change. Please add a matching assertInvalidTimestampRange(null, 0L) case.
  • Option B: leave the rule as-is and instead amend docs/en/connectors/source/Hbase.md:117 and docs/zh/connectors/source/Hbase.md:116 to say end_timestamp must be > 0, so docs, factory rule and runtime guard tell one consistent story.
• Severity: Medium

Issue 2

• Location: HbaseSourceFactory.java:64-69 (message surface), evaluated via ConfigValidator.java:238-244
• Problem description: On failure the user now sees the generic constraint rendering produced by Condition.toString() (Condition.java:233-247) wrapped by OptionUtil.formatError(), e.g. option 'start_timestamp' value validation failed: 'start_timestamp' < 'end_timestamp'. The offending values are not included. The message it replaces ("start_timestamp must be less than end_timestamp", HbaseClient.java:413) was not better, so this is not a regression — but for the #11007 migration as a whole, an error that says which constraint failed without saying what the value was is a step sideways in diagnosability.
• Potential risk: Slightly longer time-to-diagnose for users with templated/generated configs where the values are not obvious from the file.
• Best improvement: Not something to fix in this PR. Worth raising on the #11007 umbrella: ConfigValidator.collectErrors() already has config in hand and could append the actual value(s) to the TYPE_VALUE error. If you agree, a short note on #11007 would help whoever migrates the remaining connectors.
• Severity: Low

2. Code Quality Assessment

2.1 Coding Standards

• Formatting matches Spotless/AOSP; the spotless-check goal passed for connector-hbase in the fork build.
• Import is the correct non-shaded org.apache.seatunnel.api.configuration.util.Conditions, no wildcard.
• Splitting the constrained options into a second optional(...) call is the right shape given the builder API, and it reads well.
• Comment/Javadoc completeness: no new production methods or fields are introduced, so nothing is missing on the main side. On the test side, HbaseFactoryTest has no class-level Javadoc and the four new test methods carry no comment stating which production behavior they lock down. That is a pre-existing gap for the class, but this PR is where the class stops being a one-line smoke test and starts being a regression suite, so it is the natural place to add two lines. Non-blocking.

Issue 3

• Location: seatunnel-connectors-v2/connector-hbase/src/test/java/org/apache/seatunnel/connectors/seatunnel/hbase/HbaseFactoryTest.java:29-81
• Problem description: No class-level Javadoc and no comment explaining that these cases encode the [start, end) half-open contract from HbaseClient.applyTimeRange(). A future reader hitting assertInvalidTimestampRange(1000L, 1000L) has no way to know why equal timestamps are illegal without going and reading the client.
• Potential risk: Someone "fixes" the equal-timestamp case to be legal in a later refactor and silently changes scan semantics to include an empty range.
• Best improvement: Add a short class Javadoc naming the covered production behavior, e.g. "Covers HbaseSourceFactory#optionRule() timestamp constraints, which mirror the half-open [start, end) range enforced by HbaseClient#applyTimeRange", plus a one-line comment on testInvalidTimestampRangeFails noting that start == end is an empty scan.
• Severity: Medium (per the project's comment-completeness expectation for new non-trivial test surface; trivially addressed)

2.2 Test Coverage and Test Stability

Coverage is genuinely good for the change: the seven cases span absent/absent, present/absent (both directions), valid range, negative on each side, equal, and reversed. validateTimestampRange correctly builds a minimal-but-valid config (zookeeper_quorum + table) so that the required checks pass and only the value constraints are under test — that is the right isolation, and it is the detail most people get wrong when writing OptionRule tests.

Gaps worth adding while you are in here (all non-blocking):

• assertInvalidTimestampRange(null, 0L) — the Issue 1 case.
• A positive assertion on the message, e.g. assertTrue(ex.getMessage().contains("start_timestamp")), so a future refactor that reports the wrong option key is caught.
• Assertions.assertDoesNotThrow(() -> validateTimestampRange(0L, 1L)) — the minimal legal window, adjacent to the rejected 0/0.

Test stability rating: Stable.

Evidence, per the flaky-test anti-pattern checklist:

• No Thread.sleep, no polling, no timing dependence anywhere in HbaseFactoryTest.java:29-81.
• No shared static or instance state: validateTimestampRange builds a fresh HashMap and a fresh new HbaseSourceFactory() on every call (HbaseFactoryTest.java:64-80), so the tests are order-independent and parallel-safe.
• Assertions are exact (assertThrows(OptionValidationException.class, ...)), not existence checks; no floating-point comparison, so no delta concern.
• No external resource, container, network or filesystem dependency — pure in-memory config validation.
• Confirmed empirically: Tests run: 4, Failures: 0, Errors: 0 for HbaseFactoryTest in the fork's Windows unit-test job, which is the harshest environment in the matrix.

2.3 Documentation Updates

I verified your claim that no doc change is needed, and it holds:

• docs/en/connectors/source/Hbase.md:117 already states the >= 0 and start < end constraints and explains the [start, end) rationale.
• docs/zh/connectors/source/Hbase.md:116 carries the equivalent Chinese text.

So en/zh parity is intact. The one caveat is the end_timestamp = 0 wording called out in Issue 1 — whichever of Option A / Option B you pick, the doc and the rule should end up agreeing.

3. Architectural Soundness

3.1 Elegance of the Solution

This is a precise fix, not a workaround. It moves a constraint to the layer that owns constraint declaration, and — importantly — it does so additively. Keeping HbaseClient.applyTimeRange() intact is the right call: HbaseClient is also reachable from @VisibleForTesting buildScan and from programmatic construction that never goes through FactoryUtil, so deleting the runtime guard in the name of "migration" would have opened a real hole. Several migrations under #11007 will face this same temptation; this PR is a good template for how to resist it.

3.2 Maintainability

Declarative constraints are self-documenting and are surfaced to the web UI / connector-info APIs for free, which a hand-written if in the client never was. The one maintenance hazard is the duplication itself: the [start, end) contract now lives in two places (HbaseSourceFactory.java:60-70 and HbaseClient.java:403-414). That duplication is deliberate and justified here, but it is precisely why Issue 1 matters — the two copies currently disagree on one input.

3.3 Extensibility

Uses the shared Conditions DSL rather than a connector-local helper, consistent with the ~20 connectors already on this API (connector-kafka, connector-jdbc, connector-redis, connector-elasticsearch, connector-paimon, and others). No new abstraction introduced. Adding a future constraint (say a max lookback window) is a one-line addition.

3.4 Historical-Version Compatibility

• Option contract: start_timestamp / end_timestamp keys, types and (absent) defaults are untouched — HbaseSourceOptions.java:51-63 is not modified.
• Job config compatibility: per the matrix in 1.2, every config that a previous release accepted is still accepted. No previously-valid job fails to submit after upgrade.
• State / checkpoint: no serialization surface touched. HbaseSourceSplit and the source state format are unchanged, so in-flight savepoints restore cleanly; FactoryUtil.restoreAndPrepareSource (FactoryUtil.java:203) revalidates with the same rule, and since restore configs were legal before they remain legal.
• API/SPI: TableSourceFactory contract unchanged.

No historical-compatibility concern.

4. Issue Summary

#	Issue	Location	Severity
1	end_timestamp = 0 alone passes factory validation but is rejected at runtime; not full parity with applyTimeRange, and disagrees with the docs	HbaseSourceFactory.java:60-70 vs HbaseClient.java:410-414, docs/en/connectors/source/Hbase.md:117	Medium
2	Constraint-failure message omits the offending value (framework-level; raise on #11007)	ConfigValidator.java:238-244, Condition.java:233-247	Low
3	New test suite lacks class Javadoc / intent comments explaining the [start, end) contract it locks down	HbaseFactoryTest.java:29-81	Medium

CI status

The apache-side Build check is red, but neither failure is caused by this PR — I pulled both fork job logs to confirm:

1. Run / unit-test (8, windows-latest) — failed in seatunnel-engine-server, not connector-hbase:

   [ERROR]   RestApiHttpBasicTest.before:86 » IllegalState Node failed to start!
   [ERROR] Tests run: 343, Failures: 0, Errors: 1, Skipped: 20
   

   A Hazelcast member failing to bind on the Windows runner. Known-flaky, unrelated to HBase. In the same job, connector-hbase passed cleanly, including your new tests:

   Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 - in ...hbase.HbaseFactoryTest
   Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 - in ...hbase.client.HbaseClientTest
   

2. Run / Dead links — one pre-existing dead external link, https://research.google/pubs/pub43438/, referenced from the Bigtable docs. Untouched by this PR.

ubuntu-latest unit-test jobs were cancelled as collateral of the Windows failure (fail-fast matrix), so they have not actually reported yet. A rerun of the failed jobs should be enough; no code change is needed for CI.

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed

   None. The change is logically correct, fully backward compatible, exercised by deterministic tests that pass in CI, and the two red checks are unrelated infrastructure/pre-existing failures.

2. Recommended fixes — non-blocking

   1. Issue 1 — close the end_timestamp = 0 gap, preferably via Conditions.greaterThan(END_TIMESTAMP, 0L), plus an assertInvalidTimestampRange(null, 0L) case; or align the docs instead. This is the one item I would genuinely like to see before merge, because it is what makes the PR title ("Migrate ... to OptionRule") literally true.
   2. Issue 3 — add a class Javadoc to HbaseFactoryTest and a one-liner on why start == end is rejected.
   3. Add the 0/1 minimal-window positive case and an assertion on the error message content.
   4. Rerun the failed CI jobs once the above is pushed.

Overall assessment. This is a well-executed slice of #11007. The two things I specifically checked for in this class of change — (a) does the declarative rule reject anything the runtime previously accepted, and (b) does the migration delete the runtime guard — both come out clean: no new rejections, and HbaseClient.applyTimeRange is preserved for the programmatic path. The absent-option handling in ConfigValidator.isConstraintApplicable is subtle enough that I expected to find a false-positive rejection for the single-timestamp cases, and there isn't one; your test matrix covers exactly that. Nice work.

Alternative implementation note. For other connectors in the umbrella where the runtime check is more elaborate than a comparison, Conditions.extension(option, ConditionExtension) (Conditions.java:144-146) lets you keep a single source of truth by delegating to the existing validation method rather than restating it in the DSL — worth considering wherever restating the rule would risk the kind of drift described in Issue 1.

[davidzollo]

+1 if CI passes.
LGTM

[goutamadwant]

@DanielLeens updated the validation to match the existing runtime behavior. start_timestamp can be 0, but an explicitly configured end_timestamp must be greater than 0. I also added the zero-end and minimum [0, 1) cases and updated both HBase docs. The full connector package passes with 43 tests.

[goutamadwant]

@davidzollo this PR was approved but i had to push a fix and now CI is green. Can you approve and merge if no more comments? thanks!

[DanielLeens]

What Problem Does This PR Solve?

• User pain point: An invalid HBase source timestamp configuration (start_timestamp = -1, start_timestamp >= end_timestamp, etc.) was previously only rejected deep inside HbaseClient.applyTimeRange() when the Scan object is assembled — i.e. after the job has been submitted, the source has been created, splits have been enumerated, and a reader is already running. The user pays for a full submission round-trip to learn about a typo.
• Fix approach: Declare the same constraints on HbaseSourceFactory#optionRule() using the Conditions DSL, so ConfigValidator rejects the bad config at factory-validation time, before any HBase connection is opened — while preserving the existing runtime guard in HbaseClient for direct/programmatic construction paths that never go through the factory.
• One-sentence summary: Move an existing runtime precondition forward to declarative option validation, without removing the runtime precondition, and (in the commit under re-review) close the one parity gap the previous round identified between the two.

1. Code Change Review

1.1 Core Logic Analysis

This is a re-review of the current head (fec7577251e2cfa8e3c3135ba8588e02133b04e8). Daniel (DanielLeens) reviewed the prior commit a7064a1db7076fc708848e46392ca868ef1b7ac6 on 2026-08-14T10:44:29Z (COMMENTED, recommendation "Ready to merge", no blockers, three non-blocking items). I diffed a7064a1d...fec7577 directly (gh api repos/apache/seatunnel/compare/...) and confirmed the new commit ("[Improve][Connector-V2][HBase] Align timestamp validation", pushed 2026-08-15T00:26:48Z) touches exactly four files with small, surgical changes: HbaseSourceFactory.java (+1/-1), docs/en/.../Hbase.md (+1/-1), docs/zh/.../Hbase.md (+1/-1), HbaseFactoryTest.java (+7/-0). No other file changed.

Verification of each previously-raised issue against the current head:

#	Issue (Daniel, prior round)	Severity	Status now
Issue 1	end_timestamp = 0 (with start_timestamp unset) passes factory validation but is rejected at runtime — not full parity with HbaseClient.applyTimeRange, and disagreed with the docs	Medium	Confirmed resolved. See below.
Issue 2	Constraint-failure message omits the offending value (framework-level; Daniel explicitly said "not something to fix in this PR", raise on the #11007 umbrella instead)	Low	Unchanged, as intended. Not in scope for this PR; still an open framework-level recommendation, not a defect introduced or left incomplete by this PR.
Issue 3	HbaseFactoryTest had no class-level Javadoc and no comment explaining the [start, end) half-open contract the new cases lock down	Medium	Confirmed resolved. See below.

Issue 1 detail, re-verified fixed. Before (a7064a1d), the rule was:

Conditions.greaterOrEqual(HbaseSourceOptions.START_TIMESTAMP, 0L),
Conditions.greaterOrEqual(HbaseSourceOptions.END_TIMESTAMP, 0L),
Conditions.lessThanField(HbaseSourceOptions.START_TIMESTAMP, HbaseSourceOptions.END_TIMESTAMP)

After (current head, HbaseSourceFactory.java:58-65):

.optional(
        HbaseSourceOptions.START_TIMESTAMP,
        HbaseSourceOptions.END_TIMESTAMP,
        Conditions.greaterOrEqual(HbaseSourceOptions.START_TIMESTAMP, 0L),
        Conditions.greaterThan(HbaseSourceOptions.END_TIMESTAMP, 0L),
        Conditions.lessThanField(
                HbaseSourceOptions.START_TIMESTAMP,
                HbaseSourceOptions.END_TIMESTAMP))

The single-word change (greaterOrEqual → greaterThan on END_TIMESTAMP) is exactly the fix Daniel recommended as "Option A (declarative, preferred)" in his prior review. I independently re-verified parity against the runtime guard, which is unchanged by this PR: HbaseClient.applyTimeRange() (HbaseClient.java:395-416, fetched at the current head — this file is not touched by this PR) computes min = startTimestamp == null ? 0L : startTimestamp and max = endTimestamp == null ? Long.MAX_VALUE : endTimestamp, then rejects min >= max. With start_timestamp unset and end_timestamp = 0: min = 0, max = 0 → 0 >= 0 → runtime rejects. With the new rule, greaterThan(END_TIMESTAMP, 0L) now rejects end_timestamp = 0 unconditionally (its applicability segment is {end_timestamp} alone, so it fires whenever end_timestamp is present, regardless of whether start_timestamp is set) — closing exactly the gap Daniel identified. I re-walked the full accept/reject matrix and found no remaining divergence between the declarative rule and the runtime guard:

Config	HbaseClient.applyTimeRange (unchanged)	New OptionRule
neither set	accepted	accepted (both constraints skipped, absent)
start >= 0 only	accepted	accepted
end > 0 only	accepted	accepted
end = 0 only	rejected (min=0,max=0)	rejected (greaterThan(END,0) — gap now closed)
start < end, both valid	accepted	accepted
start < 0 or end < 0	rejected	rejected
start >= end, both set	rejected	rejected

There is no config accepted by one side and rejected by the other. The docs were updated in lock-step: docs/en/connectors/source/Hbase.md:117 now reads "start_timestamp must be >= 0 and end_timestamp must be > 0" (was "start_timestamp / end_timestamp must be >= 0"), and docs/zh/connectors/source/Hbase.md:116 carries the matching Chinese text — so the factory rule, the runtime guard, and both docs now tell one consistent story, which was the exact completeness gap Daniel flagged.

Runtime path (unchanged from the prior review, re-confirmed on the current head):

SeaTunnel client submits job
  -> FactoryUtil.createAndPrepareSource(...)            seatunnel-api/.../FactoryUtil.java:184-194
     -> ConfigValidator.of(context.getOptions())
          .validate(factory.optionRule())               FactoryUtil.java:191   <-- rejection point (now full parity)
     -> HbaseSourceFactory.createSource(context)
        -> HbaseParameters.buildWithSourceConfig(...)    HbaseParameters.java:168-173
  -> ... split enumeration, reader start ...
  -> HbaseClient.buildScan(split, params, columns)       HbaseClient.java:378-393
     -> HbaseClient.applyTimeRange(scan, params)         HbaseClient.java:395-416  <-- runtime guard, preserved, unreachable-but-defensive for the factory path

FactoryUtil.restoreAndPrepareSource() runs the same validation, so a savepoint/restore of an already-running job with a legal config is unaffected. This remains a main-path change (not edge/recovery-only): createAndPrepareSource is the standard source-construction path for every engine.

Issue 3 detail, re-verified fixed. Before, HbaseFactoryTest had no class Javadoc and testInvalidTimestampRangeFails had no comment on why start == end is rejected. After (current head, HbaseFactoryTest.java:79-82):

/**
 * Tests factory option rules, including the half-open {@code [start_timestamp, end_timestamp)}
 * range.
 */
public class HbaseFactoryTest {

and (HbaseFactoryTest.java:108-112):

@Test
void testInvalidTimestampRangeFails() {
    // Equal bounds describe an empty half-open range and are rejected before source creation.
    assertInvalidTimestampRange(1000L, 1000L);
    assertInvalidTimestampRange(2000L, 1000L);
}

This is a lighter-weight version of what Daniel suggested ("Covers HbaseSourceFactory#optionRule() timestamp constraints, which mirror the half-open [start, end) range enforced by HbaseClient#applyTimeRange") but captures the same substance — a future reader hitting the equal-timestamp case now has an explanation without having to go read HbaseClient. Resolved.

1.2 Compatibility Impact

Fully compatible. The externally observable contract for every already-legal config is unchanged (re-verified in the matrix above): nothing that was accepted before this PR (on dev, prior to any commit in this PR) is now rejected. The only behavioral delta from dev is when an already-illegal config is rejected (factory-validation time instead of scan-build time) and, for the newly-tightened case (end_timestamp = 0 alone), the OptionRule now catches at submission time exactly what the runtime guard already caught at scan-build time — so this closes a completeness gap without changing which configs are ultimately valid. No option was renamed, removed, or given a new default; incompatible-changes.md correctly needs no entry, and the PR description's "Does this PR introduce any user-facing change?" section discloses the behavior change accurately.

Checkpoint/restore: untouched. No serialization surface, split state, or source state format is touched by any commit in this PR.

1.3 Performance / Side-Effect Analysis

Negligible and one-shot, unchanged from the prior review's conclusion: optionRule() is built once per source construction; the extra Condition objects and evaluator lookups are on an EnumMap. No per-record, per-split, or per-checkpoint cost. No new threads, buffers, connections, or class loading.

1.4 Error Handling and Logging

No formal issues found in the code changed by this commit. The one item carried over from the prior review is explicitly out of scope for this PR by design, not left incomplete:

Issue 1 (carryover, deliberately unaddressed in this PR)

• Location: ConfigValidator.java:238-244, Condition.java:233-247 (framework-level, not this connector)
• Problem description: On failure the user sees the generic constraint rendering from Condition.toString(), e.g. option 'start_timestamp' value validation failed: 'start_timestamp' < 'end_timestamp' — the offending values are not included in the message.
• Potential risk: Slightly longer time-to-diagnose for users with templated/generated configs. Not a regression versus the message it replaces ("start_timestamp must be less than end_timestamp", unchanged in HbaseClient.java:413), which was equally value-free.
• Best improvement: Not something to fix in this connector-level PR — ConfigValidator.collectErrors() already has the raw config in hand and could append the actual value(s) to the TYPE_VALUE error at the framework level. Worth raising on the #11007 migration umbrella once, rather than per-connector.
• Severity: Low
• Raised by another reviewer: No (Daniel, prior round; carried over unchanged, by design)

2. Code Quality Assessment

2.1 Coding Standards

• Formatting matches Spotless/AOSP; CI Build (which covers this in this repo's workflow) is green on the current head.
• Import (org.apache.seatunnel.api.configuration.util.Conditions) is correct, non-shaded, no wildcard.
• Moving START_TIMESTAMP/END_TIMESTAMP out of the plain-metadata .optional(...) list into their own constrained .optional(...) call (rather than duplicating the option declaration) is the correct use of the builder API — I confirmed there is no duplicate-option registration by reading the full current HbaseSourceFactory.java: the two options appear exactly once each, now in the constrained block.
• Test class now carries a class-level Javadoc and an inline comment explaining the one non-obvious assertion (the equal-bounds case) — closes the prior gap noted in Issue 3.

2.2 Test Coverage and Test Stability

Coverage remains genuinely good and is now slightly broader: the suite spans absent/absent, present/absent (both directions), valid range, the minimal legal [0, 1) window, negative on each side, a standalone zero end (the new case), equal, and reversed. validateTimestampRange continues to build a minimal-but-valid config (zookeeper_quorum + table) so only the value constraints are under test, which is the detail most people get wrong when writing OptionRule tests.

New in this commit (HbaseFactoryTest.java, current head):

• testValidTimestampRanges gained validateTimestampRange(0L, 1L) — the minimal legal window, exactly the case Daniel recommended adding.
• testNegativeTimestampFails gained assertInvalidTimestampRange(null, 0L) — the standalone-zero-end case that is the direct regression test for Issue 1's fix. This is precisely the test that proves the parity gap is closed, not just asserted closed.

Test stability rating: Stable. Justification, per the flaky-test checklist:

• No Thread.sleep, no polling, no timing/clock dependence, no randomness anywhere in HbaseFactoryTest.java.
• No shared static or instance state: validateTimestampRange builds a fresh HashMap and a fresh new HbaseSourceFactory() on every call, so tests are order-independent and parallel-safe.
• Assertions are exact (assertThrows(OptionValidationException.class, ...) / assertDoesNotThrow(...)), not existence checks; no floating-point comparison.
• No external resource, container, network, or filesystem dependency — pure in-memory config validation.
• CI evidence on the current head: the Build check is SUCCESS (completed 2026-08-15T02:36:16Z), and the PR author's own comment states the full connector-hbase package (43 tests) passed locally as well.

One non-blocking gap remains from the prior review, not addressed by this commit and not required for merge: no test asserts on the exception message content (e.g. that it names start_timestamp), only on the exception type — this mirrors the existing style of most of the suite's other cases and is consistent with Issue 1 (2.1.4 in the prior review) being explicitly deferred to the framework level.

2.3 Documentation Updates

Both docs/en/connectors/source/Hbase.md:117 and docs/zh/connectors/source/Hbase.md:116 were updated in this commit to say end_timestamp must be > 0 (previously >= 0, which was the exact inconsistency Daniel's Issue 1 flagged against the runtime guard). I verified both language versions were updated together (en/zh parity intact), and the wording now matches the tightened Conditions.greaterThan(END_TIMESTAMP, 0L) rule and the unchanged HbaseClient.applyTimeRange runtime behavior exactly — closing the three-way disagreement (docs vs. rule vs. runtime) that existed at the prior review.

3. Architectural Soundness

3.1 Elegance of the Solution (Precise fix / Temporary workaround / Long-term solution)

Precise fix. It moves a constraint to the layer that owns constraint declaration, additively — the runtime guard in HbaseClient.applyTimeRange() is untouched, which matters because HbaseClient is also reachable from @VisibleForTesting buildScan and from programmatic construction that never goes through FactoryUtil. The follow-up commit under re-review is itself a precise, single-word fix (greaterOrEqual → greaterThan) rather than a broader rewrite, closing the one gap identified without touching anything else.

3.2 Maintainability

The [start, end) contract now lives in two places by design (HbaseSourceFactory.java:58-65 and HbaseClient.java:403-414) — a deliberate, justified duplication for early-rejection purposes. This commit removes the one place where the two copies disagreed, so the duplication is now internally consistent, which is the property that makes deliberate duplication maintainable rather than a maintenance hazard.

3.3 Extensibility

Unchanged from the prior review: uses the shared Conditions DSL consistent with ~20 other already-migrated connectors. No new abstraction introduced. Adding a future constraint is a one-line addition.

3.4 Historical-Version Compatibility

• Option contract: start_timestamp / end_timestamp keys, types, and (absent) defaults are untouched — HbaseSourceOptions.java is not modified by either commit in this PR.
• Job config compatibility: every config a previous release accepted is still accepted (matrix in 1.2/1.1); the only newly-rejected case (end_timestamp = 0 alone at submission time) was already rejected by the runtime guard on every prior release, so no previously-successful job is affected — only the timing of an already-guaranteed failure moves earlier.
• State/checkpoint: no serialization surface touched; FactoryUtil.restoreAndPrepareSource revalidates with the same (now-tightened) rule, and since restore configs were legal before, they remain legal.
• API/SPI: TableSourceFactory contract unchanged.

No historical-compatibility concern.

4. Issue Summary

Number	Issue	Location	Severity
1	Constraint-failure message omits the offending value (framework-level; deliberately out of scope for this connector PR)	ConfigValidator.java:238-244, Condition.java:233-247	Low (carryover, by design)

Previously raised and now confirmed resolved (verified against the current head, not taken on trust): end_timestamp = 0 (alone) passed factory validation but was rejected at runtime, and disagreed with the docs — fixed via Conditions.greaterThan(END_TIMESTAMP, 0L) plus matching en/zh doc updates and a direct regression test (assertInvalidTimestampRange(null, 0L)); HbaseFactoryTest lacked class-level Javadoc and an intent comment on the equal-bounds case — fixed via a class Javadoc and an inline comment.

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed

   None, in either the prior review round or this one. The prior review already had no blockers; this re-review confirms the follow-up commit correctly and completely closes the one substantive parity gap that was raised (Issue 1 in the prior round — end_timestamp = 0 alone), verified independently against the unchanged HbaseClient.applyTimeRange runtime code and both doc languages, with a direct regression test added. No new issue was introduced by the follow-up commit, which I isolated and read in full (a7064a1d...fec7577, touching only the factory rule, both docs, and the test file).

2. Recommended fixes — non-blocking

   1. Issue 1 — at the framework level (not this PR), have ConfigValidator.collectErrors() include the offending value(s) in the TYPE_VALUE error message. Worth raising once on the #11007 migration umbrella rather than fixing per-connector.
   2. Optionally add a message-content assertion (e.g. contains("start_timestamp")) to one of the negative test cases, so a future refactor that reports the wrong option key would be caught — genuinely optional, most of the existing suite already follows the type-only assertion style.

Overall assessment. The prior review already found this PR sound and recommended merge with no blockers; the one substantive item it flagged (a narrow parity gap between the declarative rule and the runtime guard for end_timestamp = 0) has now been fixed with a minimal, correct, single-condition change, matching docs, and a direct regression test that specifically proves the gap is closed rather than merely asserting it. CI (Build) is green on the current head. This is ready to merge.

[goutamadwant]

@nzw921rx whenever you get a chance - pls try to take a look at the changes and review. thanks! :)

[nzw921rx]

+1 LGTM
good job🚀