DanielLeens commented on PR #11169:
URL: https://github.com/apache/seatunnel/pull/11169#issuecomment-5340818009
This is my own PR, so please read this round as a structured maintainer
self-re-review rather than an independent assessment — GitHub blocks
self-approval here, so a write-capable maintainer still needs to make the final
call, and @SEZ9's `CHANGES_REQUESTED` review remains the operative human review
state in the sidebar. I re-checked out the current head from scratch on a fresh
worktree and re-traced the whole chain independently rather than reusing my
prior rounds' conclusions verbatim.
Reviewed head: `633a3b220e86` (the `dev`-sync merge of 2026-08-14). This is
the same head I last reviewed on 2026-08-16, and there has been no new commit
and no new reply since, so the technical conclusion below reconfirms — not
repeats blindly — that round's finding after independently re-deriving it from
the current `dev` source.
# What Problem Does This PR Solve?
- **User pain (as described in the PR title/description):** submission-time
`OptionValidationException` ("JDBC URL must contain a database name") rejecting
valid JDBC catalog/sink configs whose URL does not embed a database —
StarRocks/Doris-style `jdbc:mysql://host:9030`, SQL Server configs that supply
the database via the explicit `database` option, and Oracle thin service URLs
such as `jdbc:oracle:thin:@host:1521/ORCLCDB`.
- **Fix approach in this PR:** add a 3-arg
`JdbcCatalogUtils.findCatalog(config, dialect, database)` overload that injects
the sink's explicit `database` into the catalog `ReadonlyConfig`, wrap
`FactoryUtil.createOptionalCatalog()` in a `try/catch
(OptionValidationException)`, and skip the optional catalog
(`Optional.empty()`, falling back to direct JDBC metadata discovery) when the
raw exception message contains the literal substring `"JDBC URL must contain a
database name"`. `JdbcSink.getCatalog()` is switched to the new overload, a
StarRocks E2E assertion is loosened, and two new unit tests are added.
- **One-sentence summary:** the regression described is real *history*, but
it was already fixed directly on `dev` — independently of this branch — by
relaxing `UrlContainsDatabaseValidator` to make the database segment optional
in the URL, so on the code this PR will actually merge into, the new catch
branch is unreachable, the injected `database` option has zero consumers, and
the Oracle claim in the description doesn't correspond to anything in this diff
(Oracle already has its own dedicated, already-correct URL validator, untouched
by this PR).
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Files with the core changes:**
-
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/sink/JdbcSink.java`
(`getCatalog()`, lines ~309-322)
-
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java`
(`findCatalog(...)` overload + `extractCatalogConfig(...)`, lines 73-74,
492-542)
-
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtilsTest.java:818-908`
-
`seatunnel-e2e/.../connector-jdbc-e2e-part-2/.../JdbcStarRocksdbIT.java:118-128`
**Before (`JdbcSink.java:321`):**
```java
return
JdbcCatalogUtils.findCatalog(jdbcSinkConfig.getJdbcConnectionConfig(), dialect);
```
**After:**
```java
return JdbcCatalogUtils.findCatalog(
jdbcSinkConfig.getJdbcConnectionConfig(), dialect,
jdbcSinkConfig.getDatabase());
```
**New fallback (`JdbcCatalogUtils.java:509-522`):**
```java
} catch (OptionValidationException e) {
if (StringUtils.isBlank(database)
&& e.getRawMessage() != null
&& e.getRawMessage().contains(DATABASE_NAME_REQUIRED_MESSAGE)) {
log.info(
"Skip optional JDBC catalog for url {} because it does not
embed a database; "
+ "fallback to direct JDBC metadata discovery.",
config.getUrl());
return Optional.empty();
}
throw e;
}
```
**Complete runtime chain, independently re-verified on the current head:**
```text
Sink path
JdbcSink.getCatalog() JdbcSink.java:310-322
-> if (StringUtils.isBlank(jdbcSinkConfig.getDatabase())) return
Optional.empty(); // PRE-EXISTING guard, untouched by this diff
-> findCatalog(config, dialect, database) // database is guaranteed
NON-blank here
-> extractCatalogConfig(config, database) puts
JdbcSinkOptions.DATABASE into catalogConfig
-> FactoryUtil.createOptionalCatalog(...)
-> SqlServerCatalogFactory / OracleCatalogFactory /
MySqlCatalogFactory / ... .createCatalog()
parse the database ONLY from
options.get(JdbcCommonOptions.URL) via each dialect's
own URL parser; grep across every catalog factory in this
module shows none of them
reads JdbcSinkOptions.DATABASE — the injected key has zero
consumers.
-> the catch's `StringUtils.isBlank(database)` guard is therefore
UNREACHABLE from this call
site (database is never blank here), independent of whether the
validator still throws.
Source/legacy path
JdbcCatalogUtils.findCatalog(config, dialect)
JdbcCatalogUtils.java:492-494
-> findCatalog(config, dialect, null) (database == null, so the isBlank
guard is satisfiable
here IF FactoryUtil.createOptionalCatalog still throws the matching
message)
The validator that used to throw that message, re-checked directly on
origin/dev:
JdbcCommonOptions.UrlContainsDatabaseValidator.evaluate(...)
JdbcCommonOptions.java:200-216
-> "Database name is optional to maintain backward compatibility with
connectors (e.g.
StarRocks, Doris) that specify the database in the query or
table_path instead of the URL."
-> returns true whenever the URL parses and has a non-blank host,
regardless of whether a
database segment is present -> OptionValidationException with
"JDBC URL must contain a database name" is no longer thrown by this
validator at all.
`grep -rn "JDBC URL must contain a database name"
seatunnel-connectors-v2/connector-jdbc/src/main`
-> the ONLY occurrence repo-wide is this PR's own
DATABASE_NAME_REQUIRED_MESSAGE constant
(JdbcCatalogUtils.java:74) that the new catch branch matches against.
No validator in the
current codebase (source-side or sink-side) ever produces that text,
so the catch's
`e.getRawMessage().contains(DATABASE_NAME_REQUIRED_MESSAGE)`
condition can never be true on
this head — the entire fallback branch is dead on both call sites.
Oracle claim in the PR description, independently checked:
OracleCatalogFactory.optionRule() OracleCatalogFactory.java:58-60
-> baseCatalogRule(new OracleUrlValidator())
OracleCatalogFactory.OracleUrlValidator OracleCatalogFactory.java:62-79
-> already parses via OracleURLParser.parse(url) and requires
info.getDefaultDatabase()
to be present specifically for Oracle, independent of the generic
UrlContainsDatabaseValidator.
-> no file under .../catalog/oracle/ or .../dialect/oracle/ appears in
this PR's diff at all.
```
### Key findings
1. **The normal path does reach the changed lines** (`JdbcSink.getCatalog()`
and `JdbcCatalogUtils.findCatalog(...)` are on the standard JDBC sink/source
catalog-discovery path for every job that uses a JDBC catalog), but **the
specific `catch` branch this PR adds is not reachable by any
currently-producible exception** — I could not find any code path, on this head
or on `origin/dev`, that throws `OptionValidationException` containing the
literal text `"JDBC URL must contain a database name"`.
2. **The scenario the PR claims to fix no longer exists on the base it
merges into.** `UrlContainsDatabaseValidator` (used by the generic
`baseCatalogRule()`, i.e. MySQL/PostgreSQL/StarRocks/Doris-style dialects)
already treats the database segment as optional, and it does so directly on
`origin/dev`, not as part of this branch's own commits.
3. **The Oracle half of the PR's stated motivation does not correspond to
any code in the diff.** Oracle already has its own dialect-specific
`OracleUrlValidator` that correctly parses thin-service URLs; it was not
touched by, and does not depend on, this PR.
4. This is a **defensive/dead-code change, not a precise fix**: the intent
(tolerate DB-less URLs) is sound, but it duplicates protection that already
exists one layer up (at the `OptionRule` validator level) instead of
removing/adjusting anything there, so it adds a second, unreachable, brittle
safety net rather than restoring an actual regression.
5. The two live, observable changes in this diff are (a) an always-empty
`JdbcSinkOptions.DATABASE` key stuffed into the catalog `ReadonlyConfig` that
no catalog factory reads, and (b) a **weakened StarRocks E2E assertion** — see
Issue 4 below — that is the only part of the diff that changes what CI actually
verifies.
### In-depth correctness analysis
- **Where it "takes effect":** nowhere observable today. The sink-side call
site can never present a blank `database` to `findCatalog(...)` (pre-existing
guard), and the source-side call site can never trigger the matched exception
message on the current validator. I verified this by reading the validator
implementation directly off `origin/dev` (not just the merge-base copy embedded
in this branch), so this isn't an artifact of a stale local checkout.
- **Where it depends on state:** the catch branch would only ever fire if
some future catalog factory validator started throwing
`OptionValidationException` with that exact wording again — at which point the
branch would silently swallow it rather than surface a clear validation error,
which is a regression-in-waiting rather than a regression-fix.
- **Recovery/serialization/lifecycle impact:** none. This is a
submission-time-only validation path; no checkpoint, state, or serialization
format is touched.
## 1.2 Compatibility Impact
**Fully compatible.** No public API, `Option`, default value, protocol, or
serialization format is changed. The one new catalog-config key
(`JdbcSinkOptions.DATABASE`) is additive, has no consumer, and cannot alter
catalog-factory behavior. Historical saved job configs are unaffected either
way.
That said, "fully compatible" here also means **functionally inert** — see
Issue 1.
## 1.3 Performance / Side-Effect Analysis
Negligible. One extra `HashMap` entry per `findCatalog(...)` call and one
extra `try/catch` frame; no additional I/O, no new locks, no retries, no
resource-lifecycle change.
## 1.4 Error Handling and Logging
**Issue 1: The PR's core "fix" — the message-matched fallback and the
injected `database` option — is dead code on the base it will actually merge
into, so the PR does not restore the compatibility it claims to**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:73-74,
509-522`; validator already relaxed at
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/config/JdbcCommonOptions.java:196-217`
(present directly on `origin/dev`).
- **Problem description:** the code sits on the standard JDBC
catalog-discovery path (both sink and source), but the specific failure mode it
defends against (`OptionValidationException` with the literal text "JDBC URL
must contain a database name") is no longer producible anywhere in the current
codebase — the validator that used to throw it has already been relaxed
independently on `dev`, and Oracle (the other case cited in the PR description)
is already handled by its own dedicated validator that this PR does not touch.
- **Potential risk:** merging this as-is does not restore any behavior
(there is nothing left to restore), but it does add: (a) a second unused config
field with no consumer, (b) a brittle, string-matched catch branch that can
silently swallow a *different* future validation failure if any catalog factory
ever happens to reuse similar wording, and (c) a credential-logging branch (see
Issue 2) that is dormant today but becomes live the moment (a) is ever true
again — i.e., it's a landmine rather than a fix.
- **Best improvement:** Option A — close this PR, since the underlying
compatibility gap is already resolved on `dev` and there is no remaining defect
to fix. Option B — if there's a reason to keep defense-in-depth here (e.g.,
protecting against a future validator regression), replace the
message-substring match with a structural check (e.g., have
`UrlContainsDatabaseValidator`/`OracleUrlValidator` expose a typed "no database
in URL" signal instead of free-text, or have `findCatalog` proactively probe
`dialect`'s own URL parser for a database before calling
`FactoryUtil.createOptionalCatalog` at all), and keep only the parts of the
diff that still have live value — most likely just Issue 4's E2E tightening,
reframed as a positive regression guard rather than a loosened OR-assertion.
- **Severity:** High
- **Raised by another reviewer:** No — this specific "the target regression
is already independently fixed on dev, making the fix a no-op" framing is new
in this round, though it echoes and reconfirms the same code-path conclusion I
reached independently in my own 2026-08-10/08-13/08-16 self-review rounds on
this same branch. It is not a newly-introduced issue in this head; it has been
the standing technical conclusion since 2026-08-10 and remains true after
independent re-derivation today.
**Issue 2: Dormant credential-logging branch — raw JDBC URL logged verbatim
if the (currently unreachable) fallback ever fires**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:517-520`
(`log.info(... config.getUrl())`).
- **Problem description:** `+1` to @SEZ9's Issue 4 from the 2026-08-04/08-05
rounds — JDBC URLs frequently embed credentials
(`jdbc:mysql://host:3306/db?user=root&password=secret`, SQL Server
`;user=..;password=..`, Oracle `user/password@host`), and logging the raw URL
at INFO level risks leaking them into centralized job-submission logs on
Zeta/Flink/Spark.
- **Potential risk:** zero today (the branch is unreachable per Issue 1),
but if it ever becomes reachable — e.g., a catalog factory's validator wording
changes to coincidentally match the substring again — this becomes a live
credential leak with no test coverage to catch it.
- **Best improvement:** if this branch is kept at all (see Issue 1's Option
B), redact the URL before logging — strip query string/user/password
properties, or log only scheme+host+port.
- **Severity:** Medium
- **Raised by another reviewer:** Yes (`@SEZ9`, review `#4852487456` /
`#4861626453`, Issue 4).
**Issue 3: Fallback keyed on exception-message substring matching is
inherently brittle**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtils.java:509-522`.
- **Problem description:** `+1` to @SEZ9's Issue 3 — coupling control flow
to `e.getRawMessage().contains("JDBC URL must contain a database name")` means
any rewording of a validator's message would silently re-break the scenario
this PR targets, or silently swallow an unrelated error that happens to contain
the same phrase.
- **Potential risk:** low today since the branch is unreachable, but if
Option B in Issue 1 is taken this needs a structural fix rather than a string
match, per @SEZ9's suggestion (proactively resolve the database from the URL
via the dialect's own parser before calling
`FactoryUtil.createOptionalCatalog`, or use a typed exception/error code).
- **Best improvement:** see Issue 1, Option B.
- **Severity:** Medium
- **Raised by another reviewer:** Yes (`@SEZ9`, review `#4852487456` /
`#4861626453`, Issue 3).
**Issue 4: StarRocks E2E assertion was loosened to accept either the catalog
path or the JDBC-fallback path, reducing what the test actually proves**
- **Location:**
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-2/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcStarRocksdbIT.java:121-126`.
- **Problem description:** `+1` to @SEZ9's Issue 5 — the assertion now
accepts either `"Loading catalog tables for catalog"`
(`JdbcCatalogUtils.java:92`) or `"Loading catalog tables for jdbc"`
(`JdbcCatalogUtils.java:172`, the direct-JDBC-metadata fallback log). Since
I've independently confirmed the fallback branch is currently unreachable, this
OR-relaxation adds slack without a matching increase in what actually gets
exercised — a future regression that silently downgraded StarRocks from the
dedicated catalog path to the generic JDBC path would still pass this test.
- **Potential risk:** reduced regression-detection power for the StarRocks
catalog-loading path specifically.
- **Best improvement:** assert the single expected log line for this
scenario (`"Loading catalog tables for catalog"`), matching the pre-PR
assertion, unless there's a concrete reason StarRocks is expected to sometimes
take the JDBC-fallback branch in this test's configuration.
- **Severity:** Low
- **Raised by another reviewer:** Yes (`@SEZ9`, review `#4852487456` /
`#4861626453`, Issue 5).
**Issue 5: New unit tests only validate the message-matching logic against a
hand-crafted mock exception, not the real, currently-reachable catalog-factory
behavior**
- **Location:**
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/utils/JdbcCatalogUtilsTest.java:844-905`
(`testFindCatalogFallsBackWhenUrlOmitsDatabase`,
`testFindCatalogPropagatesOtherValidationFailures`).
- **Problem description:** both tests use
`Mockito.mockStatic(FactoryUtil.class)` to force `createOptionalCatalog(...)`
to throw a hand-built `OptionValidationException` with the exact literal
message the production code matches on. Since I confirmed no real
catalog-factory validator in the current codebase ever produces that message,
these tests will keep passing forever regardless of whether the catch branch is
truly exercisable by any real dialect — they prove the `String.contains(...)`
logic works in isolation, not that the scenario in the PR description is
actually reachable or fixed.
- **Potential risk:** low/non-blocking on its own, but it's part of why this
PR's "the fix works" self-assessment (and my own earlier rounds' initial
passes) went unchallenged for as long as it did — mocked-exception tests can't
reveal that the exception is never thrown in practice.
- **Best improvement:** in addition to (or instead of) the mocked-exception
unit tests, add a test that drives the real
`UrlContainsDatabaseValidator`/`OracleUrlValidator` +
`FactoryUtil.createOptionalCatalog` end-to-end with a DB-less URL and asserts
on the actual outcome, which would have surfaced that the scenario no longer
throws.
- **Severity:** Medium
- **Raised by another reviewer:** related to `@SEZ9`'s Issue 1 (review
`#4861626453`), but a different point — see the correction below.
**Correction to @SEZ9's Issue 1 (High, "missing static imports /
mockito-inline not available") — I could not reproduce this on the current
head, with evidence:**
- The `import static org.mockito.ArgumentMatchers.any;` / `.eq;` static
imports @SEZ9's Issue 1 says are missing are actually already present at lines
61-62 of `JdbcCatalogUtilsTest.java` **before this PR's diff** — I confirmed
this by reading the file directly at this PR's own merge-base commit (`git show
<merge-base>:.../JdbcCatalogUtilsTest.java | grep 'static org.mockito'`), so
the file compiles under the module build; this isn't specific to this PR's
added code.
- `mockito-inline` (which provides the inline mock-maker required for
`Mockito.mockStatic(...)`) is declared as a real, inherited test-scope
dependency in the root `pom.xml`'s own `<dependencies>` block (not just
`<dependencyManagement>`) at `pom.xml:605-609`, applied to every Maven module
including `connector-jdbc` — `connector-jdbc/pom.xml` doesn't need its own
explicit `mockito-inline` entry.
- I'd respectfully downgrade Issue 1 from a High blocker to not-applicable
on the current head; the underlying test-quality gap it was gesturing at is
better captured by Issue 5 above (mocked exception vs. real reachable
behavior), which I've raised separately with different evidence.
**Correction to the framing of @SEZ9's Issue 2 ("sink-side catalog is now
silently skipped... a behavior regression from the previous fail-fast
validation"):**
- The specific "silent skip when database is blank" behavior on the sink
side predates this PR — `JdbcSink.getCatalog()`'s `if
(StringUtils.isBlank(jdbcSinkConfig.getDatabase())) return Optional.empty();`
guard (lines 310-312) is unchanged context in this diff, not a new line. This
PR doesn't introduce or change that guard; it only changes which overload is
called once `database` is already known to be non-blank. The underlying
architectural point (should a JDBC sink fail fast when it can't resolve a
catalog?) may still be worth discussing, but it isn't something this diff
regresses, so I wouldn't hold this PR responsible for it.
**Issue 6 (Low): PR description's Oracle claim doesn't match the diff**
- **Location:** PR description ("Oracle thin service URLs such as
`jdbc:oracle:thin:@host:1521/ORCLCDB` were checked only by the generic JDBC URL
parser.") vs. the actual 4-file diff, which touches no file under
`.../catalog/oracle/` or `.../dialect/oracle/`.
- **Problem description:** Oracle already has its own dedicated
`OracleCatalogFactory.OracleUrlValidator` (`OracleCatalogFactory.java:62-79`)
that correctly parses thin-service URLs via `OracleURLParser`, independent of
`UrlContainsDatabaseValidator`. If this claim was accurate at some earlier
point in the branch's history, it no longer matches what will actually be
merged.
- **Potential risk:** low — mostly a documentation-accuracy /
changelog-accuracy concern, but it makes it harder for a reviewer or future
reader to trust the PR description's problem statement.
- **Best improvement:** update the PR description to describe only what the
current diff actually does, or drop the Oracle claim if it's no longer
applicable.
- **Severity:** Low
- **Raised by another reviewer:** No.
# 2. Code Quality Assessment
## 2.1 Coding Standards
The new `findCatalog(config, dialect, database)` overload has a Javadoc
comment; the new `DATABASE_NAME_REQUIRED_MESSAGE` constant and the
injected-`database` branch in `extractCatalogConfig` are self-explanatory
enough given their names, though given Issue 1 above, a short comment noting
the intended defense-in-depth purpose (and that it currently guards against a
scenario the validator no longer produces) would help the next reader avoid the
same confusion this review had to untangle from scratch. No AOSP/Spotless
formatting issues observed in the diff.
## 2.2 Test Coverage and Test Stability
**Coverage:** the new unit tests cover the `String.contains(...)` branch
logic in isolation (see Issue 5) but not the real, currently-reachable
catalog-factory validation behavior. The StarRocks E2E was loosened rather than
tightened (Issue 4).
**Mandatory stability analysis (Section 5.10.2), since this PR changes UT
and E2E code:**
- **a) Intermittent-failure risk:** none observed. The two new unit tests
use `Mockito.mockStatic` inside a `try`-with-resources block, scoped per test,
with no shared static state left behind; no timing, no `Thread.sleep`, no
environment-dependent assertions. The relaxed E2E assertion is a plain
synchronous string-match on already-captured stdout — no new timing dependency
introduced.
- **b) Flaky-test anti-patterns:** none of the catalogued anti-patterns
(hard waits, unreleased resources, unreset statics, order-dependence,
floating-point/string-normalization issues, weak readiness signals) apply to
the diff. `MockedStatic` is closed automatically by try-with-resources in both
new tests.
- **c) Stability rating: Stable.** No flakiness risk from this diff; the
coverage concern in Issue 5 is a correctness/precision gap, not a stability gap.
## 2.3 Documentation Updates
No `docs/en` or `docs/zh` update is required — no new user-facing `Option`
is introduced (`JdbcSinkOptions.DATABASE` already existed), and no documented
default or behavior changes for a real, reachable scenario. If Issue 6 is
addressed by narrowing the PR description, that's PR-description hygiene rather
than `docs/` content.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Given the current base, this is closest to a **temporary workaround that has
outlived its target** — it was presumably written against an earlier state of
`dev` where the validator still rejected DB-less URLs, and the branch's own
subsequent `dev`-sync merges pulled in the independent fix, leaving this PR's
protection stranded and unreachable.
## 3.2 Maintainability
The added code is small and self-contained, so it isn't a maintenance burden
by size, but an unreachable catch branch with a hardcoded message constant is
exactly the kind of thing that erodes trust in "why is this here" over time,
and the dormant credential-logging risk (Issue 2) makes it a poor thing to
leave lying around even if harmless today.
## 3.3 Extensibility
N/A beyond what's covered above — this doesn't establish a reusable pattern
that other dialects would want to follow (each dialect already owns its own URL
validator).
## 3.4 Historical-Version Compatibility
No impact either way — fully compatible per 1.2, and since the change is
functionally inert on the current base, there's nothing for older jobs, saved
configs, or upgrade paths to be incompatible with.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | Core fix is dead code on the base it merges into; PR doesn't restore
anything | `JdbcCatalogUtils.java:73-74,509-522`;
`JdbcCommonOptions.java:196-217` | High |
| 2 | Dormant credential-logging branch (raw JDBC URL) |
`JdbcCatalogUtils.java:517-520` | Medium |
| 3 | Brittle exception-message substring matching |
`JdbcCatalogUtils.java:509-522` | Medium |
| 4 | Weakened StarRocks E2E assertion (OR of two log lines) |
`JdbcStarRocksdbIT.java:121-126` | Low |
| 5 | New unit tests only validate mocked logic, not real reachable behavior
| `JdbcCatalogUtilsTest.java:844-905` | Medium |
| 6 | PR description's Oracle claim doesn't match the diff | PR description
| Low |
# 5. Merge Recommendation
### Conclusion: Not recommended for merge
1. **Blockers — must be fixed**
- Issue 1 (High): the PR's stated purpose — restoring JDBC catalog
database compatibility — is already accomplished on `dev` independently of this
branch. Merging this as-is doesn't fix anything; it only adds unreachable,
brittle code plus a dormant credential-logging branch and a weakened regression
test. Before this can merge, the PR needs to either (a) be closed as
no-longer-needed, since the target regression is already resolved, or (b) be
substantially narrowed to keep only genuinely live value (most plausibly,
tightening rather than loosening the StarRocks E2E assertion, and/or adding the
real end-to-end test suggested in Issue 5) with the dead validation-catch code
and its associated risk (Issues 2–3) removed.
2. **Recommended fixes — non-blocking**
- Issue 4 (Low) and Issue 6 (Low) are worth fixing if any part of this PR
is salvaged, but they don't independently block anything — they'd be resolved
automatically if Issue 1's Option A (close) is taken, and easy one-line fixes
if Option B (narrow and keep) is taken instead.
**Overall assessment:** the investigative work behind this PR (identifying
that CI jobs like the SQL Server XA schema-evolution workflow were breaking on
`OptionValidationException`) was legitimate and valuable, and I want to be
clear this isn't a case of sloppy work — it's a case of the target moving out
from under the fix during a long-lived branch's multiple `dev`-sync merges,
which is an easy trap to fall into on any PR that stays open across several
weeks of upstream activity. @SEZ9's review rounds correctly flagged the
substring-matching brittleness, the credential-logging risk, and the loosened
E2E assertion well before I did; I'm largely reconfirming and building on that
work here, with one correction (Issue 1 of that review, re: missing
imports/mockito-inline, doesn't reproduce on this head) and one addition (the
fix's core branch being unreachable at all, which is the more fundamental
reason to not merge this as-is).
**Alternative:** Option A (close the PR, since `dev` already has the fix) is
my preferred path unless there's a still-live scenario I'm missing where the
database-in-URL validation can still fail with this exact message on some
dialect I haven't checked — if so, please point me at it and I'll re-verify
that specific path. Option B (narrow to a tightened StarRocks E2E assertion
plus a real end-to-end regression test, dropping the dead validation-catch
code) is the fallback if there's a reason to keep this open.
---
**CI / merge-gate facts as of this review:**
- `Build` check: `SUCCESS` (completed 2026-08-15T23:37:31Z) — this is a
draft PR, but CI did run and is currently green, so there is no CI-side blocker
right now.
- `mergeStateStatus`: `BLOCKED` / `mergeable_state`: `blocked`,
`reviewDecision`: `REVIEW_REQUIRED` — consistent with the draft state and the
still-open review-request gate, not a merge conflict (`mergeable: true`).
- Compare vs. `dev`: `status=diverged`, `ahead_by=9`, `behind_by=28`. This
is queue metadata rather than an active blocker — CI is currently green, so
there's no "CI failure that might be resolved by syncing" scenario here; the
28-commit drift is exactly what let the target validator fix land on `dev`
without this branch's own commits, which is central to Issue 1 above rather
than a separate CI concern.
--
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]
