fatmanverse commented on PR #11436:
URL: https://github.com/apache/seatunnel/pull/11436#issuecomment-5010822442
Thanks again for the careful re-review — really appreciate the runtime-chain
writeup, it made each point easy to follow. Pushed a follow-up (`f563743`).
Here's where each item stands:
**Issue 2 (interface check) — addressed.** `S3HadoopConf` now asserts the
resolved class `isAssignableFrom(com.amazonaws.auth.AWSCredentialsProvider)`
and rejects abstract classes before writing the property. I cross-checked this
against `hadoop-aws 3.1.4` (the version this repo pins, AWS SDK v1): these are
the same two checks `S3AUtils.createAWSCredentialProvider` performs before
instantiation (`NOT_AWS_PROVIDER` / `ABSTRACT_PROVIDER`), so the eager check
should never reject anything Hadoop itself would accept — it just surfaces the
failure at config-parse time with a clearer message. To stay safe under
plugin-classloader isolation, the assertion is skipped when the AWS interface
isn't visible from the same classloader that resolved the provider, so a valid
provider is never falsely rejected.
**Issue 3 (chain splitting) — addressed.** Splitting now uses `split(",",
-1)` + `trim()` to mirror Hadoop's `getTrimmedStrings` semantics: `"A, B"` is
accepted, while `"A,,B"` and a trailing `"A,"` are rejected as empty entries.
The class-not-found path is a WARN rather than a hard failure, and it now
explicitly says the jar must be present on **every** node. I added
`S3HadoopConfTest` cases for `"A, B"`, `"A,,B"`, `"A,"`, chain pass-through, a
non-provider entry within a chain, and an abstract class.
**Issue 1 & 4 (enum removal / binary compatibility) — just want to make sure
we're looking at the same state.** The `S3aAwsCredentialsProvider` enum is
still present — it was restored as `@Deprecated` in `ef5ce98`, with its
constants delegating to the new `String` constants as a single source of truth,
so the `NoClassDefFoundError` risk for downstream code should be covered. The
remaining change is the option's generic type
(`Option<S3aAwsCredentialsProvider>` → `Option<String>`), which unfortunately
can't coexist as two same-named fields; that's documented as a code-level (not
just config-level) break in `incompatible-changes.md` (en + zh) with migration
notes, and I confirmed no other module — including the Zeta checkpoint-storage
S3 path — still references the removed generic type. If there's a specific
angle on the binary-compatibility concern I've missed, I'd be glad to dig into
it further.
On the CI failures: I looked into these and they appear unrelated to the S3
change. `unit-test` fails in `seatunnel-engine-server`'s `MetricsApiTest` (an
environment-dependent test that binds port 8080 / a Hazelcast HTTP endpoint),
and `integration-test-part-5` fails in `FtpFileIT` (a Testcontainers FTP e2e
that gets pulled in only because this PR touches a shared file-connector base
module). Neither exercises the S3 credentials path, and this branch doesn't
modify those modules. Happy to re-trigger a run.
Verified locally: `spotless:apply` clean, and the `connector-file-s3` unit
tests (including the new chain/interface/abstract cases) all pass. Thanks again
for the guidance on this.
--
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]
