SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6392496104
Thanks for the quick follow-up, @sudeephazra. Since my last pass at `6080bcde6123`, the only new commit is `80b1635222` (head `80b16352229d`), and it touches only `ADLSConfigValidator.java`, which matches what you described. On the `hadoop_adls_properties` denylist in `ADLSConfigValidator.java`: the seven new prefixes cover the class-loading and token-provider ABFS keys that `rejectsCredentialAndClassLoadingAdvancedProperties` expects, and the trimmed, lower-cased `startsWith` handles the per-account suffix and padded/upper-case variants, so that part looks right. One thing to settle before approval: the same hunk dropped the `fs.s3` prefix from the reject list. Nothing in the commit message or this thread indicates that was intentional. Could you either restore `fs.s3` (ideally with a test assertion) or explain why removing it is correct? Noted on `unit-test` passing on Ubuntu Java 11/17 and the Windows leg failing for an unrelated reason; I will not block on Windows. Remaining items from my earlier review, as a checklist: 1. Restore (or justify removing) the `fs.s3` prefix in `ADLSConfigValidator.java`, with test coverage. 2. The validator trims option values but `ADLSHadoopConf.java` consumes raw values, so whitespace from env-var substitution passes validation and then fails later; please apply the normalized values in `ADLSHadoopConf.java` or reject untrimmed values up front. 3. The OAuth token endpoint in `ADLSRuntimeCompatibility.java` is built from unvalidated `authority_host`/`tenant_id`; please require a secure scheme and a sane host/tenant shape so the client secret cannot be POSTed over plain HTTP or to an arbitrary URL. 4. `docs/en/connectors/sink/ADLSFile.md`: add the HNS (hierarchical namespace) requirement behind the transactional `tmp_path` -> `path` rename, document the validation rules users will hit (account/container naming, auth mutual exclusion, blocked `hadoop_adls_properties` keys), fix the shipped template placeholders so they pass those rules, and bring the en/zh option tables in line with what the factories expose. 5. `docs/en/connectors/source/ADLSFile.md`: correct the `discovery_mode` and `start_mode` defaults to `ONCE` and `EARLIEST` to match the code and the Chinese doc. 6. Mention the `ConfigShadeUtils.java` change adding `account_key` to the default log-mask list in the docs and PR description, since it affects all connectors. If you would prefer to split any of the docs items into a follow-up PR, just say so and I am happy to track them separately; items 1-3 I would like in this PR. <!-- streview-comment:2013 --> -- 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]
