loustler commented on PR #11648:
URL: https://github.com/apache/seatunnel/pull/11648#issuecomment-5186777161
Thank you for the review — three of the four blockers were things I would
not have found on my own, and you were right that `-am` walks dependencies
rather than dependents, which is exactly why my verification missed Issue 1.
Pushed an update. All numbers below are measured on the new head. Doing what
you asked also turned up four defects that changed the PR itself, not just its
description — they have their own sections after the six issues.
## Issue 1 — stale `seatunnel-hadoop3-3.1.4-uber` in
`seatunnel-engine-examples`
Fixed: **artifactId rename only**, no classifier or scope. You were right to
ask — every other consumer uses `<classifier>optional</classifier>` +
`<scope>provided</scope>`, but this module deliberately wants the main
artifact, which the shade plugin still produces. `git grep
'seatunnel-hadoop3-3\.1\.4-uber' -- . ':!docs'` is now empty (the two
`incompatible-changes.md` matches name the old artifactId on purpose, as the
"renamed from" side).
## Issue 2 — license bookkeeping
Fixed by running the gate and reflecting its output rather than editing by
hand. Delta: **36 removals / 39 additions**.
| change | n | cause |
|---|---:|---|
| awssdk `2.31.30` → `2.35.4` | 26 | the SDK version bump, via
`transforms-v2` |
| `netty-*` `4.1.118.Final` → `4.1.126.Final` | 10 | netty is managed by the
AWS SDK BOM |
| `okhttp-2.7.5.jar`, `okio-1.6.0.jar` | +2 | §5; okio 1.6.0 is what okhttp
2.7.5's parent pom pins — resolved, not assumed |
| `utils-lite-2.35.4.jar` | +1 | did not exist in the 2.31.30 closure |
`checkLicense.sh` ends in a **two-way** `diff` under `set -e`, so the stale
awssdk entries would have failed it even with okhttp added. The netty rename is
the part inspection would miss: **no netty version appears anywhere in this
diff.** Gate now exits 0 with an empty diff.
Also added OkHttp 2.7.5 and Okio 1.6.0 to `release-docs/LICENSE` — neither
ships as a standalone jar, but `seatunnel-starter.jar` bundles 184
`com/squareup/okhttp/` and 35 `okio/` classes.
Worth recording: the gate never sees `seatunnel-shade`
(`<skip>${e2e.dependency.skip}</skip>`, default `true`) or `seatunnel-dist`
(`-pl '!seatunnel-dist'`), which is why the Hadoop bump and the SDK narrowing
add no license work at all. `release-docs/LICENSE` has also drifted from the
gate file independently of this PR (all 26 awssdk and all 10 netty filenames
have no counterpart there) — happy to open an issue if that's worth tracking.
## Issue 3 — `hadoop-aliyun`
The analysis came back **dirty**, so this PR moves `hadoop-aliyun` to 3.4.3
in lockstep, unified onto one root property across all five call sites.
Same method as for `hadoop-aws` — resolve every external `Class` /
`Methodref` / `InterfaceMethodref` / `Fieldref` against the target with
descriptor matching and hierarchy walking, with `<init>` resolved only against
its declaring class since constructors are never inherited (JVMS 5.4.3.3):
| `hadoop-aliyun` | vs `hadoop-common` | unresolved |
|---|---|---|
| 3.1.4 | 3.1.4 *(control)* | **0** |
| 3.1.4 | **3.4.3** | **1** |
| 3.0.0 | 3.0.0 *(control)* | 0 |
| 3.0.0 | 3.4.3 | 0 |
| **3.4.3** | **3.4.3** | **0** |
The one break is the same one that forced `hadoop-aws` to move —
`AliyunOSSFileSystem` calling a constructor 3.4.3 replaced:
```
hadoop-common 3.0.0 SemaphoredDelegatingExecutor does not exist
hadoop-common 3.1.4
<init>(Lcom/google/common/util/concurrent/ListeningExecutorService;IZ)V
hadoop-common 3.4.3 <init>(Ljava/util/concurrent/ExecutorService;IZ)V (+
a DurationTrackerFactory variant)
```
**On the blast radius:** the compile surface is nil. Repo-wide there is
exactly one compile-time reference to a `hadoop-aliyun` type —
`OssHadoopConf.java:23` reads three `Constants` fields; everything else names
`AliyunOSSFileSystem` as a string. Those fields are `public static final
String` with identical values at both versions (`fs.oss.accessKeyId`,
`fs.oss.accessKeySecret`, `fs.oss.endpoint`). I compared the *values*, not just
the names, because javac folds constant variables into the caller's constant
pool (JLS 13.1) — a renamed key would have left the connector silently writing
the old config key with nothing visible at build time.
**One correction to the premise:** `hadoop-aliyun` declares `hadoop-common`
as **`compile`**, not `provided` (`hadoop-aws` is `provided` at both versions).
The coupling is real but arrives differently — SeaTunnel declares
`hadoop-aliyun` `provided` and ships only that jar, so it links at runtime
against the uber jar's `hadoop-common`. Same conclusion.
**On the evidence you wanted:** there is no OSS E2E module in this
repository and MinIO speaks S3 rather than the Aliyun OSS protocol, so I can't
offer one — only the analysis with matching control runs showing the break
comes from the version gap and not from the method. Glad to build an OSS E2E as
a follow-up.
## Issue 4 — distribution size
Done: `seatunnel-hadoop-aws.jar` **~658 MB → 24,187,329 B (23.1 MB)**, below
even the ~83 MB it was at on 3.1.4.
I kept `bundle` with a path allowlist rather than switching to individual
modules, and the deciding factor was risk rather than size.
**Size is a wash.** `hadoop-aws` 3.4.3 references 3 of the bundle's 411
service packages — `s3` (1248 refs), `sts` (82), `kms` (2 symbols) — with
**zero** reflective service lookups, and no `dynamodb` since S3Guard was
removed in 3.4.0 (my PR description was wrong to list it). The allowlist keeps
16,585,287 B and drops 564,461,442 B; the individual-module closure is 34
artifacts / 11,219,489 B plus BOM-managed externals (netty ≈ 4.5 MB,
httpclient+httpcore ≈ 1.1 MB, …) ≈ 16.5 MiB, versus 15.8 MiB filtered.
**The measured difference.** `ConfigureShadedAWSSocketFactory` references
`software/amazon/awssdk/thirdparty/org/apache/http/conn/socket/ConnectionSocketFactory`
and `…/conn/ssl/SSLConnectionSocketFactory` — FQCNs that exist **only inside
`bundle`**; standalone `apache-client` brings plain `org.apache.http`.
`NetworkBinding` loads that class reflectively and swallows the failure, so
individual modules would not fail the build — they would quietly turn
`fs.s3a.ssl.channel.mode` into a no-op. They would also need a
`ServicesResourceTransformer` (each module ships its own `SdkHttpService` /
`SdkAsyncHttpService` file; shade's default is last-one-wins) and would put
unrelocated `org.apache.http`, `io.netty`, `org.reactivestreams` and
`org.slf4j` at the root of a `lib/` jar.
Two deliberate details: it is an **allowlist** because shade's
`SimpleFilter` is `!(isIncluded && !isExcluded)` — an exclude can never be
re-included, and an allowlist can't silently regrow as AWS adds services. And
all 25 non-service packages are kept rather than hand-picked (4.3 MB total;
`imds`, for one, is reached reflectively via `IAMInstanceCredentialsProvider`).
The entire saving comes from `services/`.
Verified on the built jar: service packages exactly `kms`/`s3`/`sts`; `imds`
intact; both relocated socket-factory classes present; root `org/slf4j/impl/**`
and `org/apache/log4j/**` empty while the SDK's own 16 relocated slf4j entries
survive.
**Happy to switch to individual modules if you still prefer it** after these
numbers — it's a contained change.
**You were more right here than my first fix was.** The distribution *also*
shipped the raw bundle as a standalone `lib/` jar:
`assembly-seatunnel-bin-ci.xml` had `com.amazonaws:aws-java-sdk-bundle` and my
first pass simply swapped it for `software.amazon.awssdk:bundle` — 86,220,098 B
becoming **686,119,507 B** — so the built distribution carried a filtered *and*
an unfiltered copy of `software/amazon/awssdk/**` on one classpath. The include
was already redundant before this PR, which is why it's dropped rather than
swapped: `seatunnel-hadoop-aws` shades `hadoop-aws` with its whole SDK
unrelocated (the released 2.3.13 jar holds 42,579 `com/amazonaws/` classes),
`assembly-seatunnel-bin.xml` — the release descriptor — never included a
standalone bundle, and nothing can depend on a **v2** SDK from `lib/` because
`dev`'s `lib/` had none (`transforms-v2` already shades its own). Dropped along
with its `provided` declaration in `seatunnel-dist/pom.xml`: **−686,119,507 B**
on t
he CI distribution, release tarball unchanged.
## Issue 5 — okhttp 2.x
Option A, as you recommended. The 2.7.5 declaration stays; the PR
description now records explicitly that this newly *ships* a 2016-era EOL
library where SeaTunnel previously only borrowed the classes from Hadoop.
Follow-up: **#11656** — one main file plus two tests, and I'd appreciate your
review on it.
## Issue 6 — version-neutral artifactId
Keeping `seatunnel-hadoop3-3.4.3-uber` for now, taking you at your word that
it's a project naming convention rather than your call to make unilaterally and
doesn't block merge. I'd rather not churn another ~56 files in a PR that
already had four blockers — but your reasoning is right, and if another
maintainer weighs in for `seatunnel-hadoop3-uber` I'll do it here, which would
also mean `incompatible-changes.md` documents one rename instead of two.
---
## Defect 1 — a shipped `NoClassDefFoundError` the Hadoop bump introduced
Found only because you insisted on the `hadoop-aliyun` analysis, and the
strongest argument for the position you took.
`WALReader` and `DefaultReader` in `imap-storage-file` call
`CollectionUtils.isEmpty` / `isNotEmpty` from
**`org.apache.commons.collections`** (3.x), which no pom in their ancestry
declares. It arrived two ways at once and the bump removes both: on the compile
classpath via `hadoop-aliyun` → `hadoop-common` (compile scope), and at runtime
from inside the shaded uber jar. 3.4.3 declares `commons-collections4` instead,
and the built uber jar has **zero** `org/apache/commons/collections/` entries.
Both classes ship inside `seatunnel-starter.jar`, and the shipped bytecode
holds an unrelocated `invokestatic
org/apache/commons/collections/CollectionUtils.isEmpty` — verified by
disassembly, with no other jar on the core runtime classpath carrying the class.
So the Hadoop bump alone would have produced a `NoClassDefFoundError` on the
IMap file-storage WAL read path (cluster restart, IMap recovery with the file
backend) with **nothing failing at build time**. Moving `hadoop-aliyun` to
3.4.3 removed the compile-time transitive too, which is what turned it into a
loud compile error.
Fixed by switching both imports to `commons-collections4` (identical
null-safe semantics; all four call sites pass a `List`) and declaring it
explicitly at the root-managed version, as the sibling
`checkpoint-storage-local-file` already does. No license change — that jar is
already known.
## Defect 2 — the #11436 credentials-provider validation had silently
stopped working
`S3HadoopConf` validates `fs.s3a.aws.credentials.provider` at config-parse
time by resolving two classes: the configured provider, and the provider
interface named as a string — `"com.amazonaws.auth.AWSCredentialsProvider"`,
exactly what this PR stops shipping. `checkCredentialsProviderClass` needs
**both** to resolve before checking assignability and `continue`s otherwise, so
the whole thing falls through to its closing `LOGGER.warn` for **every**
provider value, including correct v2 ones — while telling the user their
provider "could not be fully validated" and that "the job will fail at
runtime". No exception, no failing test.
It now names
`software.amazon.awssdk.auth.credentials.AwsCredentialsProvider`, which is what
S3A itself checks (`CredentialProviderListFactory` references it, and the
narrowed jar carries that class and no
`com/amazonaws/auth/AWSCredentialsProvider`). The unit test's
`AbstractTestCredentialsProvider` moved to the v2 interface.
I found it via a test-compile error, which exposed a problem in my own
process: `-Dmaven.test.skip=true` skips test *compilation*, so I was reporting
a green full-reactor `install` while a test source in my change set did not
compile. `install -DskipTests` from now on.
## Defects 3 and 4 — found by the MinIO E2E you asked for
`connector-file-s3-e2e` is green: **BUILD SUCCESS**, `S3FileWithFilterIT` 2
tests / 0 failures / 0 errors in 31.6 s, thread-leak check included.
(`S3FileIT` and `S3FileWithMultipleTableIT` are `@Disabled("have no s3
environment to run this test")`.) Two failures on the way there were real
defects rather than flakes.
**The harness was injecting the very mismatch this PR removes.** All three
ITs downloaded `hadoop-aws-3.1.4.jar` and `aws-java-sdk-bundle-1.11.271.jar`
from Maven Central into the container, so both tests failed with
`IllegalArgumentException: The S3A credentials provider class
'org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider' does not implement
'software.amazon.awssdk.auth.credentials.AwsCredentialsProvider'` — 3.1.4's
provider implements the v1 interface. What caught it was the #11436 validation
repaired above; on the head you reviewed it would have fallen through to a
warning and failed later inside S3A, far less legibly. Fixed by copying the
built `seatunnel-hadoop-aws.jar` into `lib/` instead of downloading anything —
how `SeaTunnelContainer` already handles the uber jar, and what a real
distribution looks like. `S3FileWithFilterIT` moved from `buildStartCommand()`
to `executeExtraCommands(GenericContainer)`, which runs before `server.start()`.
**Then the assertions passed and the thread-leak check failed.** AWS SDK v2
leaves two daemon threads behind, traced by scanning the shipped jars' constant
pools for the literal names: `response-input-stream-timeout-scheduler`
(`software/amazon/awssdk/core/ResponseInputStream$TimeoutScheduler`) and
`FailsafeDelayScheduler` (the shaded Failsafe inside the S3 Access Grants
plugin). Both are lazily created, not owned by an `SdkClient` — so closing the
filesystem can't shut them down — and both are daemon threads parked on a delay
queue, so they can't hold the JVM open. Added to `isSystemThread`, the same
treatment that method already gives
`java-sdk-progress-listener-callback-thread`, the SDK **v1** analogue. If you'd
rather see the SDK shut down properly than allowlisted, say so — I couldn't
find a hook that isn't owned by client lifecycle.
**What the green run does not cover:** static credentials only, so no
`IAMInstanceCredentialsProvider` (the changed default), no STS, no assumed
roles. And it does **not** validate the assembly change — the harness never
deploys the dist tarball; `ContainerUtil`/`SeaTunnelContainer` assemble the
container from module `target/` directories. I assumed otherwise at first and
want to correct it: dropping the standalone bundle rests on the three static
grounds in Issue 4, not on this test.
**One thing I did not change, and would like your call on:**
`connector-hive-e2e` has the same stale downloads (`hadoop-aws/3.1.4`,
`hadoop-aliyun/3.1.4`, `aliyun-sdk-oss/3.4.1`, `hadoop-cos`) into
`/tmp/seatunnel/plugins/Hive/lib`. Only the four
`testFakeSinkHiveOn{HDFS,S3,OSS,Cos}` tests use that directory and all four are
`@Disabled`, so I can't verify a change to it. Happy to do it here if you
prefer that to a follow-up.
---
## `incompatible-changes.md`, with a correction to my own claim
My first draft said any explicit `com.amazonaws.auth.*` value fails with
`ClassNotFoundException`. That's wrong for five of them:
`CredentialProviderListFactory` holds a fixed `V1_V2_CREDENTIAL_PROVIDER_MAP`,
consulted **before** the class is resolved, that rewrites
| configured v1 name | remapped to |
|---|---|
| `com.amazonaws.auth.InstanceProfileCredentialsProvider` |
`org.apache.hadoop.fs.s3a.auth.IAMInstanceCredentialsProvider` |
| `com.amazonaws.auth.EC2ContainerCredentialsProviderWrapper` |
`org.apache.hadoop.fs.s3a.auth.IAMInstanceCredentialsProvider` |
| `com.amazonaws.auth.EnvironmentVariableCredentialsProvider` |
`software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider`
|
| `com.amazonaws.auth.profile.ProfileCredentialsProvider` |
`software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider` |
| `com.amazonaws.auth.AnonymousAWSCredentials` |
`org.apache.hadoop.fs.s3a.AnonymousAWSCredentialsProvider` |
each with a `LogExactlyOnce` warning. So those five keep working, and the
docs now say so.
That also reframes the `S3FileBaseOptions` default: the old value is the
first row above, mapping to exactly the `IAMInstanceCredentialsProvider` this
PR now names — so the edit is a **cleanup** (dropping a dependency on a shim
that warns on every use and is documented as transitional), not a rescue from a
hard break. PR description corrected.
Everything outside those five does fail, and the docs now describe it
concretely rather than as `ClassNotFoundException`: S3A logs `Failed to create
{} as v2 credentials, trying to instantiate as v1`, falls back to its v1-to-v2
adapter, and fails with `AWS V1 SDK is not on the classpath`, because the
adapter needs `com.amazonaws:aws-java-sdk-core` which `hadoop-aws-3.4.3.pom`
declares `provided`. `com.amazonaws.auth.ContainerCredentialsProvider`, which
earlier S3File docs offered as an example, is in this group. The docs also
point at `fs.s3a.aws.credentials.provider.mapping` as the escape hatch for
arbitrary v1 names.
## Two pre-existing items swept up
Both instances of the same drift this PR argues against:
`imap-storage-file/pom.xml` pinned `hadoop-aws` at 3.1.4 in a module-local
property (now inherited from root — consistency rather than a live break, since
that module holds no compiled reference to a `hadoop-aws` type), and
`connector-hudi/pom.xml` carried a dead `<hadoop-aws.version>` property
(removed).
## CI
You noted Issues 1 and 2 would both surface as `Build` failures. There was a
third: **`code-style` would have failed too** — `spotless:apply` reformatted a
javadoc block in `S3FileBaseOptions.java` that I'd touched in the previous push
without running spotless. Now clean.
Gates run locally before pushing: full-reactor `install -DskipTests`
(284/284 BUILD SUCCESS, test sources compiled), `checkLicense.sh` (exit 0,
empty diff), root `spotless:check` (exit 0), and the S3 MinIO E2E above.
## Your answers to my questions
- **#9599** — understood; leaving the `Closes` line for a maintainer with
write access.
- **SDK v1 in the two E2E modules** — agreed, follow-up. Test scope only, no
effect on the shipped artifact.
- **`JsonSerialization.getMapper()`** — opened as **#11655**, with three
possible directions. I haven't measured which is best and would welcome a
preference before I put a PR together.
Thanks again for the depth here. Please re-review whenever convenient — and
if any of the measurements above look wrong to you, I'd rather hear it than
have it merged.
--
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]
