loustler commented on PR #11673:
URL: https://github.com/apache/seatunnel/pull/11673#issuecomment-5209517588
@DanielLeens thanks — both findings are fixed in `fb7697e2f`.
**Issue 1.** Went with your Option A (`seatunnel-shade/**` into
`api_files`), and deliberately not Option B, though I started out leaning the
other way. The framing that changed my mind is that "more precise" and "correct
verification scope" are not the same thing here.
Under Option B, a change confined to `seatunnel-shade/**` would run exactly
two things: my static Jackson check and the license check. No runtime coverage
at all. But a relocation rewrites bytecode in jars that every connector loads
at runtime, and the static check only asserts one invariant — that Hadoop
classes carry no original-Jackson references. Everything else a bad relocation
can break — reflection, `META-INF/services`, some other descriptor — only shows
up when the code actually runs. This PR series already has a case in point: the
`okio` relocation in #11648 broke `seatunnel-engine-k8s-e2e`, and nothing but
E2E caught it.
So the condition you attached to Option B ("if `seatunnel-shade/**` changes
shouldn't otherwise imply *api changed*") is the crux, and I think it doesn't
hold — the shaded jars *are* the runtime surface for every connector, so a
shade change is an api change.
On the cost: `git log --since='2 years ago' -- seatunnel-shade/` is 15
commits out of 1785, under 1%. Paying full coverage seven or eight times a year
is a fair trade for a change class with this blast radius. Side effect worth
naming: this PR now expands its own CI to the full matrix, since it touches
`seatunnel-shade/` itself.
One detail your analysis didn't cover, which slightly narrows the gap but
doesn't close it: `hadoop3.version` is declared in three places that must move
in lockstep —
```
seatunnel-shade/seatunnel-hadoop3-3.1.4-uber/pom.xml:29
seatunnel-core/seatunnel-starter/pom.xml:31
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-iceberg-s3-e2e/pom.xml:31
```
`seatunnel-core/**` is already in `api_files`, so a *correct* Hadoop bump
would have tripped the gate anyway. But that's incidental rather than
guaranteed, which is exactly your point — a minimal diff touching only the uber
pom would still have slipped through.
**Issue 2.** The mask now keys on the full `org/apache/seatunnel/shade`
prefix instead of the bare word `shade`, named as a constant that points back
at the pom property so the coupling is visible:
```python
# Mirrors ${seatunnel.shade.package} in the root pom. ...
SHADED_PREFIX = rb"org[/.]apache[/.]seatunnel[/.]shade"
RELOCATED = re.compile(SHADED_PREFIX +
rb"[A-Za-z0-9_$/.]*?(?:com[/.]fasterxml[/.]jackson)")
```
Re-ran the controls after narrowing it, since a mask change is exactly the
kind of edit that can quietly turn the check into a rubber stamp:
| artifact | result |
|---|---|
| patched hadoop-aws 3.1.4 | PASS — 194 scanned |
| unpatched hadoop-aws 3.4.3 | FAIL — 6 of 466 |
| missing file | FAIL |
| non-zip input | FAIL |
Also confirming the ordering fix from the previous commit worked: on the
last run the `Check shaded Jackson references` step passed, and `Check
Dependencies Licenses` passed after it.
--
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]
