DanielLeens commented on PR #12293:
URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-5881992666
Thanks @sudeephazra for the detailed line-by-line confirmation, and @SEZ9
for keeping the tracking tight.
I independently re-verified each of these seven points against the current
head (`8d0953c941c4`), which is unchanged from `904107e329c9` for every ADLS
file — the only commits since my last approval are two `Merge branch 'dev'`
commits touching unrelated modules (engine-ui, other connectors, docs for other
connectors, CI tooling); zero files under `connector-file-adls*` or
`docs/*/connectors/{source,sink}/ADLSFile.md` changed.
- **Trim mismatch**: confirmed fixed. `ADLSConfigValidator.required()`
returns `value.trim()` (`ADLSConfigValidator.java:126-132`), and
`ADLSHadoopConf.buildWithReadOnlyConfig()` routes every field (account,
container, endpoint, account_key, authority_host, tenant_id, client_id,
client_secret) through `required()` rather than reading `config.get(...)`
directly (`ADLSHadoopConf.java:64-92`). No raw/untrimmed read remains.
- **HTTPS/tenant validation before the OAuth token endpoint is built**:
confirmed. `ADLSRuntimeCompatibility.clientCredentialsOptions()` calls
`normalizeAuthorityHost()` and `validateTenantId()` (`:133-134`) before
constructing `fs.azure.account.oauth2.client.endpoint.<host>` (`:142-144`), and
`normalizeAuthorityHost()` (`:188-206`) rejects any non-`https` scheme, user
info, non-root path, query, or fragment. `client_secret` can't reach a
non-HTTPS or malformed endpoint.
- **`account_key` log-mask**: confirmed present in
`ConfigShadeUtils.DEFAULT_LOG_MASK_ONLY_KEYWORDS`
(`ConfigShadeUtils.java:57-59`).
- **HNS requirement in sink docs**: confirmed, documented right next to the
exactly-once/atomic-rename claim (`docs/en/connectors/sink/ADLSFile.md:41-42`).
- **`discovery_mode`/`start_mode` EN defaults**: confirmed fixed to
`ONCE`/`EARLIEST` in the English source doc, matching the code and the ZH doc.
- **`hadoop_adls_properties` denylist**: confirmed it now blocks the
credential / OAuth-provider / SAS / delegation-token / identity /
shell-key-provider / routing / S3 key families described
(`ADLSConfigValidator.java:107-117`).
- **Option tables (en/zh alignment)**: almost there — one small residual gap
I found: the English sink table lists a `name` option row that the Chinese sink
table doesn't have. Low-severity; it can ride along with the
already-tracked/acknowledged zh-docs native-speaker pass rather than needing
its own round.
One CI note, since the head moved since my last review: the two dev-sync
merges brought along some CI red on this exact head — `unit-test (11,
ubuntu-latest)` fails on `SavePointTest.testSavePointButJobGoingToFail` (an
engine checkpoint-timing test), and there's also a Windows `unit-test` failure
and a `transform-v2-it-part-1` failure. I traced all three and none touch
anything under `connector-file-adls*`; they're pre-existing flakes/timing
issues picked up from `dev`, not caused by this PR. Worth a rerun once `dev`
settles, but not blocking on the ADLS side.
Nothing else outstanding from my side — my approval on `904107e329c9`
stands, and it applies cleanly to `8d0953c941c4` too since nothing in this PR's
own files changed. Thanks again for the thorough closeout, both of you.
--
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]
