davidzollo commented on PR #11671:
URL: https://github.com/apache/seatunnel/pull/11671#issuecomment-5202144453
Went through the diff independently as well (architecture, scheduling math,
sink/checksum aggregation, tooling, CI) and reached the same overall read as
the review posted earlier today: nothing blocking for a Phase-1, opt-in tool.
Two things worth adding that weren't covered there.
**1. `LatencyHistogram.percentile()` silently clamps during the exact
overload scenario the docs recommend creating**
`LatencyHistogram`
(`seatunnel-benchmarks/src/main/java/org/apache/seatunnel/benchmark/connector/sink/LatencyHistogram.java`)
buckets latencies into a fixed `maxTrackedMillis + 2`-slot array (default
`maxTrackedMillis = 60_000`), with the last slot as an overflow bucket:
```java
long percentile(double percentile) {
...
for (int index = 0; index < counts.length; index++) {
seen += counts[index];
if (seen >= rank) {
return index;
}
}
return maximum;
}
```
When the requested percentile's rank falls in the overflow bucket, this
returns the bucket **index** (60001), not the true latency, which can be far
higher during severe backlog. `getMaximum()` tracks the real unbounded max
correctly, but
`p50`/`p95`/`p99`/`first_half_p99`/`second_half_p99`/`latency_growth_ratio` all
go through `percentile()` and can silently understate.
That lines up badly with the doc's own guidance ("start above expected
capacity and lower the rate" / the "Evaluate the Result" table) — that workflow
is specifically designed to push into this overflow regime.
`latency_overflow_rows` is exposed in the JSON so a careful reader could catch
it, but nothing in `regression_report.py`'s `validity_text()` or the docs'
interpretation guidance ties the two together. There's also no dedicated unit
test for `LatencyHistogram` — the one indirect exercise
(`BenchmarkSinkWriterTest`) uses ~5ms latencies, nowhere near this path.
Suggest either surfacing overflow distinctly (don't let the clamped value
read as a real millisecond figure) or explicitly gating `sustainable`/report
rendering on `overflow == 0`, plus a one-line callout in the "Evaluate the
Result" doc table.
**2. The untrusted PR-comparison build shares the job's Maven cache with the
trusted baseline build**
In `benchmarks.yml`, `actions/setup-java` is configured once with `cache:
maven`, and the same job then runs `mvn package` against both `baseline/`
(trusted) and, when `pr_number` is set, `candidate/` — an arbitrary fork's
`pom.xml`. Maven plugin execution during `package` is arbitrary JVM code with
the job's full filesystem access, so a malicious `pom.xml`/plugin binding in
the candidate build could write into `~/.m2/repository` before `setup-java`'s
post-job hook persists that cache, which could then be restored into a later
trusted run depending on GitHub's cache-key/restore-key matching. This sits on
top of (not instead of) the "runs arbitrary PR code" trust boundary already
flagged above — the specific new wrinkle is that the cache gives a poisoned
artifact a path to outlive the single job run. `contents: read` and no
`secrets:` usage cap the blast radius (no credential exfiltration), so I'd call
this a cheap-to-fix hardening item rather than a confirmed exploit: disable
`cache: maven` for this workflow, or give the candidate build its own
`-Dmaven.repo.local`.
Neither of these changes the overall verdict — still nothing blocking for
Phase 1.
--
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]
