fatmanverse commented on PR #11436: URL: https://github.com/apache/seatunnel/pull/11436#issuecomment-5011920947
@SEZ9 I reworked the review fixes in `fcfcf014634d`; this comment supersedes my previous response. **Issues 1 and 4 - public API compatibility** The fix now preserves the original public API exactly, rather than only restoring the enum type: - `S3A_AWS_CREDENTIALS_PROVIDER` remains `Option<S3aAwsCredentialsProvider>` with the same key and default. - The deprecated `S3aAwsCredentialsProvider` enum remains available. - A separate `Option<String> S3A_AWS_CREDENTIALS_PROVIDER_CLASS` uses the same stable config key for the new free-form path. - All in-repo production consumers (S3 source/sink factories, Hive, Redshift, and `S3HadoopConf`) now use the new string option. The legacy field is only a compatibility surface and no longer owns runtime behavior. - The EN/ZH incompatible-change entry was removed because downstream code no longer needs recompilation or migration. I also added a compile-time regression assertion by assigning `ReadonlyConfig.get(S3A_AWS_CREDENTIALS_PROVIDER)` directly to `S3aAwsCredentialsProvider`. `javap` confirms that both public generic signatures are present. **Issue 2 - provider validation and classloader isolation** Each candidate classloader must now resolve both the configured class and `AWSCredentialsProvider` before assignability is checked. If an isolated TCCL resolves only one side, validation continues with the connector classloader instead of returning early. Resolvable non-provider and abstract classes fail fast; locally unresolvable classes remain a warning because the provider is instantiated on worker JVMs. **Issue 3 - Hadoop chain semantics** The validator now calls Hadoop 3.1.4 `StringUtils.getTrimmedStrings` directly. This matches Hadoop for comma/newline separators, whitespace, and a trailing comma, while still rejecting an interior empty entry such as `A,,B`. Tests cover whitespace, newline validation, trailing comma parity with `Configuration.getClasses`, interior empty entries, and the isolated-TCCL bypass. **Docs** All four EN/ZH source/sink rows now document the four creation mechanisms supported by Hadoop 3.1.4: `(URI, Configuration)`, `(Configuration)`, static no-arg `getInstance()`, and a no-arg constructor. They also retain the interface, provider-chain, every-node classpath, and shared-cluster security notes. **Local verification** - `S3HadoopConfTest,S3FileFactoryTest`: 17 tests passed - `S3StorageTest`: 2 tests passed - `connector-s3-redshift`: package passed - `./mvnw spotless:apply`: passed - `./mvnw -q -DskipTests verify`: passed across the full reactor I also attempted the root `./mvnw test`; it stops before the affected modules in `seatunnel-config-shade` with 7 unrelated `NoClassDefFoundError` failures for shaded Typesafe Config classes. The PR CI run for this head is now queued. -- 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]
