SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6486114456
Thanks @sudeephazra for the per-finding walkthrough and for pointing me at e3d5c7b148399fc61797c28d403c12d189a2a7ab. I haven't yet verified the changes in that commit, so I'm treating all of the points below as pending verification rather than resolved until I've gone through the diff: - **PR12293-F1** – trimming of both keys and values in `ADLSHadoopConf.java:71`, with the coverage you mention in `ADLSHadoopConfTest.java:119` and `ADLSConfigValidatorTest.java:70`. That is the fix I was after; I'll confirm against the diff. - **F2** – HNS / same-container rename prerequisites in both sink references (English sink:39). - **F3** – naming, auth mutual exclusion and blocked-property rules in the references, plus the naming-valid placeholders in `config/v2.batch.adls.config.template:39`. - **F4** – partial is acceptable given the compatibility constraint. When I review `ADLSRuntimeCompatibility.java:133` I'll specifically check that a non-HTTPS `authority_host` is rejected (not just that HTTPS ones are accepted), and that the "no host allowlist, operator selects the credential recipient" limitation is stated in the user-facing references, not only in `PR_DESCRIPTION.md:39`. - **F5** – en/zh sink tables aligned with the factory options (English sink:52, Chinese sink:37). - **F6** – docs part (English sink:138 and mirrors) looks like what I asked for. Separately, please also put the wording you prepared in `PR_DESCRIPTION.md:62` into the actual PR description on GitHub so the global masking change is visible there. - **F7** – ONCE / EARLIEST defaults (English source:74, :76). - **F8** – expanded denylist in `ADLSConfigValidator.java:99` with coverage in `ADLSConfigValidatorTest.java:117`, plus the documented non-exhaustive scope. One small ask: the screenshot (5ae8a121 / 80b9a79fd01e) isn't searchable — could you paste the relevant text as a code block so it's reviewable in the thread? I'll follow up once I've gone through the commit. <!-- streview-comment:2107 --> -- 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]
