SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-6029508630
Thanks @sudeephazra for splitting the follow-ups into fragments — much easier to track. **F2 (HNS requirement)** – Thanks for pointing to the sink prerequisite section at cbe10d8098f001db5a50dd321a68a4df67419828. Requiring HNS for transactional commit / exactly-once, explaining the partial-output risk without it, and requiring `tmp_path` and `path` to share a container is exactly what I was after. I'll confirm against the revision diff. **F3 (undocumented validation rules / template)** – Documenting the naming, auth mutual-exclusion and blocked `hadoop_adls_properties` rules, plus fixing the template placeholders, addresses the concern as described. Fine that users still supply their own account key. I'll confirm against the diff. **F4 (OAuth endpoint construction)** – Validating `tenant_id` and requiring an HTTPS authority with a valid host (rejecting user info, non-root paths, queries and fragments) sounds like it resolves the plain-HTTP / arbitrary-URL concern. I'm fine with not enforcing a trusted-host allowlist as long as custom HTTPS authority hosts are an intentional, supported feature — please make sure the `authority_host` option description states that explicitly so users understand the trust boundary. I'll confirm the rest against the diff. **F7 (source doc defaults)** – Thanks; I'll verify that both source docs show `discovery_mode = ONCE` and `start_mode = EARLIEST` in the revision. Still open from the previous review — could you confirm the status of each (either a pointer to the fix in the current revision, or a note if you disagree)? - **F1** – `ADLSHadoopConf` consuming raw (untrimmed) option values while the validator checks trimmed ones, so whitespace from env-var substitution passes validation and then fails at runtime. Ideally both paths should use the same normalized values. - **F5** – Option tables in the sink docs missing options the factories actually expose, and the en/zh sink tables having drifted from each other. - **F6** – The `ConfigShadeUtils` change adding `account_key` to the default log-mask list is not mentioned in the docs or the PR description; please call it out since it affects all connectors. - **F8** – `hadoop_adls_properties` prefix denylist still allowing class-loading and token-provider ABFS keys through, which contradicts the "non-routing, non-credential" contract in the option description. Either tighten the denylist or soften the documented contract. Once those are covered I'll do a final pass on the full revision. <!-- streview-comment:1568 --> -- 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]
