DanielLeens commented on PR #11169: URL: https://github.com/apache/seatunnel/pull/11169#issuecomment-5350373788
Follow-up self-check, ~16 hours after my last comment on this same head (`633a3b220e86`, unchanged — no new commit, no new discussion since). Rather than repeat the full write-up, I re-verified live the one fact the whole conclusion rests on: is the target validator on `dev` still database-optional (making this PR's catch branch unreachable)? - Pulled `dev`'s current `JdbcCommonOptions.UrlContainsDatabaseValidator#evaluate` directly: it only checks `StringUtils.isNotBlank(urlInfo.getHost())` — no database-name requirement at all. That confirms Issue 1 still holds: `dev` doesn't just have an independent fix for the exact symptom this PR targets, it doesn't require a database in the URL in the first place, so the `OptionValidationException`-with-that-message path this PR's catch branch is written for can't be produced by the current validator on this base. The PR's core change remains dead code. - **CI** — still fully green (`Build`, `Notify test workflow`, `labeler` all pass on this SHA). - **Merge state** — still `mergeStateStatus=BLOCKED` / `mergeable=MERGEABLE` (draft/review-required gate, not a conflict). Divergence from `dev`: `ahead_by=9` (unchanged), `behind_by=32` (was 28) — the gap that let the independent fix land without this branch is still widening. ### Conclusion: Not recommended for merge (unchanged) Same as yesterday: Issue 1 (High, core fix unreachable/dead on current base) is still the blocker, with the dormant credential-logging branch (Issue 2) and brittle substring matching (Issue 3) as reasons not to keep the dead code around even if harmless today. My preference is still Option A — close this PR, since `dev` already resolves the database-in-URL scenario independently and more thoroughly (host-only validation, not just a rescued error message) — unless there's a still-live failure mode on this exact base I haven't found; happy to re-check a specific dialect/URL shape if one is pointed out. -- 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]
