SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6406747818
Thanks @sudeephazra for the follow-up and for the pointers. On the two items from the earlier round, taking your responses as given: - **F1 (trimmed values vs. ADLSHadoopConf):** If `ADLSHadoopConf.java:61` is now consuming the trimmed values returned by `ADLSConfigValidator.required()` for the routing and authentication options, that closes the gap I was concerned about (whitespace from env-var substitution passing validation and then failing later). One remaining ask: please confirm that the optional/non-required options that flow into the Hadoop configuration go through the same normalized path, or that they are not sensitive to whitespace, so there is no second path where a raw value can slip through. - **F2 (HNS requirement for exactly-once):** The sink prerequisite at `docs/en/connectors/sink/ADLSFile.md#L39` tying transactional commit/exactly-once to HNS, and requiring `tmp_path` and `path` in the same container, is exactly what I was after. Please make sure the Chinese sink page carries the equivalent statement so the two do not drift. On `fs.s3`: understood, thanks for clarifying that it was a leftover from a different test and was removed on purpose. No objection from my side; I only ask that the validator test still exercises at least one blocked-prefix key so the denylist behavior stays covered. Thanks also for updating the blocked-key lists in the four ADLSFile reference pages in d283237afd9a3ecec490f4788ffa84b89395dc31 and for the doc prefix/punctuation fixes in 92e5a6094d5acede93fb7b8d43b72ca17ccef208. I will re-read those pages against `ADLSConfigValidator.java` to confirm the documented prefixes now match what the code actually blocks. I will also keep an eye on the run at https://github.com/sudeephazra/seatunnel-adls-connector-v2/actions/runs/40191709609. Remaining from the earlier review, where I have not yet seen a response, could you give a short status (done / won't fix with rationale) on each: - **F3** – documenting the validation rules (account_name/container naming, auth mutual exclusion, blocked `hadoop_adls_properties` keys) and fixing the template placeholders that violate them. - **F4** – validating `authority_host`/`tenant_id` before building the OAuth token endpoint in `ADLSRuntimeCompatibility.java` so `client_secret` cannot be POSTed over plain HTTP or to an arbitrary host. - **F5** – bringing the option tables in line with what the factories expose and re-syncing the en/zh sink tables. - **F6** – mentioning the `ConfigShadeUtils.java` change (adding `account_key` to the default mask list) in the docs and PR description. - **F7** – correcting the English source doc defaults for `discovery_mode` and `start_mode` to match the code (and the Chinese doc). - **F8** – either tightening the `hadoop_adls_properties` denylist for class-loading / token-provider ABFS keys or adjusting the documented "non-routing, non-credential" contract to match the actual behavior. Once those are addressed or answered I am happy to do a final pass. <!-- streview-comment:2033 --> -- 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]
