SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5862393420
@luozihen Thanks for the detailed report and for the retries. The failure in
`all-connectors-it-2 (8, ubuntu-latest)` / `all-connectors-it-2 (11,
ubuntu-latest)` — not being able to pull the
`quay.io/minio/minio:RELEASE.2024-06-13T22-53-53Z` image — looks like an
image-pull problem in the test environment rather than something caused by this
PR's changes. Since eight retries gave the same result, it doesn't look like a
simple transient blip, so there's no need to keep retrying for now. I'll take a
look at that side and re-trigger those jobs once it's sorted; we won't block
the PR on a purely environmental error.
In the meantime, the review items that still need attention on the PR itself
are:
- **F1 / F7 (ReadonlyConfig.toConfig())** — the JSON round-trip changes how
dotted keys are expanded for every caller of `toConfig()`, not only the JDBC
sink. Please either scope the change so existing callers keep the old behavior,
or add a regression test in `ReadableConfigTest` that pins the previous
dotted-key expansion and shows why the new behavior is safe.
- **F2 / F5 (declaration-order matching)** — the option is a `Map<String,
Object>`, so "first pattern in declaration order wins" isn't guaranteed. Either
switch to an ordered structure (e.g. a list of pattern entries) or update
`Jdbc.md` to describe the actual matching rule.
- **F3 (PRIMARY_KEYS into SQL)** — resolved key column names are
comma-joined and end up in generated SQL; please validate them as identifiers
(or check them against the table schema) before they reach the SQL builder.
- **F4 (`${primary_key}` / `${unique_key}` vs. engine TablePlaceholder)** —
please clarify in code/docs which layer expands these placeholders and what
happens when both apply.
- **F6 (regex validation)** — compile user-supplied patterns up front and
map invalid patterns to the new `JDBC-12` error code with a clear message.
- **F7 (E2E)** — add at least one engine-level IT/E2E case exercising the
new option end-to-end.
- **F8 (option name)** — `multi-table_config` mixes hyphen and underscore;
please rename to a consistent snake_case key.
Once those are pushed I'll re-run CI and do another pass.
<!-- streview-comment:1371 -->
--
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]
