SEZ9 commented on PR #11169: URL: https://github.com/apache/seatunnel/pull/11169#issuecomment-5421604191
@DanielLeens Thanks for the deep trace — and apologies for the delayed reply. To answer your core point directly: no, the end-to-end proof against real (non-mocked) code has **not** been done yet, and I agree it is a blocker. Your finding that the injected `database` key is never read by any real `CatalogFactory` (F3) and that no production validator throws the exact message the catch block matches on (F1) means both halves of the mechanism need to be reworked, not just tested harder. Concrete remaining asks before this can move forward, all within the existing review scope: 1. **F1/F3 (mechanism):** Replace the message-substring match in `JdbcCatalogUtils.java` with a typed/structured check, and make the catalog factory actually consume the injected `database` key — then demonstrate the SQL Server explicit-`database` case passing against real factories, not mocks. 2. **F2/F7 (silent degradation):** The fallback to `Optional.empty()` must not silently disable schema-save-mode/auto-create behind an INFO log, and a blank `database` with a database-less URL should fail fast with a clear config error rather than deferring to runtime write failures. 3. **F5 (log hygiene):** Redact or drop the raw JDBC URL from the new fallback log line — it can carry embedded credentials. 4. **F6 (test):** Restore a strict assertion in `JdbcStarRocksdbIT.java`; the `catalog` OR `jdbc` check can mask exactly the regression this PR is trying to prevent. 5. **F8 (compile):** Add the missing `ArgumentMatchers` static imports in `JdbcCatalogUtilsTest.java`. 6. **F4 (docs):** Add a docs/changelog entry for the validation behavior change, and either include the Oracle thin URL handling in the diff or remove that claim from the PR description. Once an updated revision addresses these — ideally with an E2E run exercising the SQL Server explicit-`database` path against real code as you asked — I'm happy to re-review promptly. <!-- streview-comment:570 --> -- 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]
