fatmanverse commented on PR #11436:
URL: https://github.com/apache/seatunnel/pull/11436#issuecomment-5001891879
Thanks for the thorough re-review — the runtime call-graph writeup made each
point easy to reason about. Pushed a follow-up commit (`ef5ce98`). Here's how
each item was addressed, plus one regression your Issue 3 analysis helped me
catch.
**Issue 1 (blocking) — binary compatibility.** Good catch on the type
removal. To be precise about the break: the `S3A_AWS_CREDENTIALS_PROVIDER`
field descriptor is actually unchanged (both `Option<Enum>` and
`Option<String>` erase to the same descriptor), so field references stay
binary-compatible; the only real removal was the `S3aAwsCredentialsProvider`
enum *type* itself. I've restored that enum as `@Deprecated` (its constants now
reference the new `String` constants, single source of truth), which removes
the `NoClassDefFoundError` risk for any downstream code compiled against it.
The option stays `stringType()` since that's the core of the feature. The
source-level generic change is documented in
`docs/{en,zh}/introduction/concepts/incompatible-changes.md` with migration
guidance.
**Issue 3 — check runs on the conf-building node, not workers. This turned
out to be a real bug, not just a robustness concern.** Your analysis was
exactly right, and it was already breaking CI: the original eager
`Class.forName` check *hard-failed*, and because the default provider
(`InstanceProfileCredentialsProvider`) lives in `seatunnel-hadoop-aws` at
`provided` scope, it isn't on the classpath of the module that builds the conf
in `connector-hive`'s unit tests — so `S3StorageTest` was failing on the
*default* config (this is the `unit-test` job that was red). I've changed the
check to **best-effort**: a class that cannot be resolved on the conf-building
node is now only logged as a `WARN` (with the "must be present on every node
running S3A" hint) and passed through, never rejected. `S3StorageTest` is green
again.
**Issue 2 — interface check.** Kept a *fast-fail* only for the case that can
never work on any node: a class that *does* resolve but does not implement
`com.amazonaws.auth.AWSCredentialsProvider` is rejected with an actionable
error. The check also splits Hadoop-style comma-separated provider chains and
validates each entry. The interface assertion is skipped when the AWS SDK
interface itself doesn't resolve from the same classloader (plugin-classloader
isolation), to avoid falsely rejecting a valid provider. On the **allowlist**:
I'd prefer to leave it out of this PR. It's a new operator-facing config with
its own compat surface, and the same class-name-loading vector already exists
for any `*.class` key passed through `hadoop_s3_properties` — singling out this
one option would be inconsistent. Happy to file a separate issue if a general
"class-name option allowlist" is worth pursuing cluster-wide.
**Issue 4 — docs.** Updated all four rows (en/zh × source/sink): the class
must implement the AWS credentials-provider contract and expose a no-arg or
`(URI, Configuration)` constructor; comma-separated chains are accepted; the
jar must be on **every** node; and a short security note that this option lets
job authors load classes by name on shared/multi-tenant clusters.
On the runtime `ClassNotFoundException` wrapping and a dedicated Zeta e2e
(Issue 3 follow-ups): I've left those out on purpose. The runtime path goes
through `HadoopFileSystemProxy`, which is shared by all file connectors, so
wrapping it there is a cross-connector change beyond this PR's scope; and a
full Zeta e2e feels heavy for a non-blocking item. Both make sense as separate
follow-ups if you'd like.
Verified locally: `spotless:apply` clean, `connector-file-s3` unit tests
pass, and `connector-hive` `S3StorageTest` passes again. Thanks again for the
careful review.
--
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]
