SEZ9 commented on PR #12299:
URL: https://github.com/apache/seatunnel/pull/12299#issuecomment-5674519143
Thanks @DanielLeens for reconciling the two reviews, and @goutamadwant for
the High findings — a fix covering both sets is now pushed as `a9612322a` (18
files, +588/-8). Below is what changed for each item, and what I did and did
not verify.
## The two High findings
**H1 — the `size <= maxBytes` fast path called the unbounded
`readFileToStr()`.** Confirmed as described: with a configured limit above ~2
GB, a file between 2 GB and that limit took the fast path and was read whole,
reintroducing the OOM this PR exists to prevent. The limit is now clamped
before it is used for anything:
```java
/**
* The largest tail {@link #readFileTailToStr(Path, long)} can return. A
byte array cannot hold
* more than {@link Integer#MAX_VALUE} entries and some JVMs reserve a few
of those for the
* array header, so a limit above this one cannot be honoured however much
heap is available.
*/
public static final long MAX_TAIL_BYTES = Integer.MAX_VALUE - 8;
public static long effectiveTailLimit(long maxBytes) {
return Math.min(maxBytes, MAX_TAIL_BYTES);
}
```
and the fast path now compares against the clamped value (`channel.size() <=
keep`) rather than the raw `maxBytes`, so an over-large configured limit
degrades to the largest tail a single read can actually return instead of to
"no limit at all".
**H2 — `tailFromLineStart` returned empty when the window's only `\n` was
the file's trailing terminator.** Confirmed by inspection before fixing: for a
10-byte window ending in `\n`, the old code found the break at index 9 and
returned `Arrays.copyOfRange(bytes, 10, 10)` — an empty array. This is
precisely the shape a log whose last entry is a large stack trace produces, so
the endpoint would have returned nothing exactly when it was most needed. The
scan now stops one byte short, with the reason recorded where the off-by-one
lives:
```java
private static int lineStartOffset(byte[] bytes, int length) {
// A '\n' on the final byte is the terminator of the line before it, not
the start of
// another line, so it is not a boundary to align to. Stopping short of
it is what keeps a
// window holding a single newline-terminated line - the shape produced
by a log whose last
// entry is a large stack trace - from being reported as empty.
for (int i = 0; i < length - 1; i++) {
if (bytes[i] == '\n') {
return i + 1;
}
}
// A single line longer than the limit leaves no boundary to align to,
so drop just the
// leading UTF-8 continuation bytes to avoid starting in the middle of a
character.
int start = 0;
while (start < length && (bytes[start] & 0xC0) == 0x80) {
start++;
}
return start;
}
```
The UTF-8 fallback matters for the same reason: a single overlong line has
no break to align to, and truncating mid-character would emit a replacement
character at the start of every such response.
## The Medium/Low items
- **Charset inconsistency (Issue 1).** Both early-return paths now decode
UTF-8 via a private `readFileToUtf8Str`, so the same file no longer decodes
differently depending only on its size. `readFileToStr(Path)` is deliberately
left on the platform default charset — it has unrelated callers and changing it
is a wider blast radius than this PR should carry.
- **Extra copy in `tailFromLineStart` (Issue 5).** Gone. The method is now
`lineStartOffset`, returning an offset that is handed straight to `new
String(tail, start, length - start, UTF_8)` — one allocation instead of three.
- **Duplicated `maxLogResponseBytes()` (Issue 8).** Rather than just
deduplicating the arithmetic, I moved the meaning of the option into
`HttpConfig.getLogResponseMaxSizeBytes()` and put the whole read behind one new
class, `LogContentReader`, that both the v1 processor and the v2 servlet call.
The two endpoints can no longer disagree about the cap, the marker, or the
charset, because there is only one code path left.
- **Silent truncation.** A response that was cut now says so, on its own
first line, and names the option to change:
```
[SeaTunnel] Log truncated: returning the last 67108864 bytes of 214748364,
starting at the first complete line. Raise
seatunnel.engine.http.log-response-max-size-mb, or set it to 0 for no limit, to
return more.
```
Deliberately a literal `\n` rather than `%n`, so the output is
platform-independent and testable.
- **Docs (Issues 2/4/6).** `incompatible-changes.md` gains an entry under
`### Engine Behavior Changes` in both `docs/en` and `docs/zh`, naming both
endpoint pairs and `log-response-max-size-mb: 0` as the escape hatch.
`rest-api-v1.md` gains a "Response Size Limit" section in both languages,
cross-referencing the v2 page. `rest-api-v2.md` no longer hard-codes "last 64
MB" — it names the option — and documents the two details a user will otherwise
file a bug about: the returned tail is slightly smaller than the limit because
it starts at a line boundary, and an overlong single line is aligned to a
character boundary instead. Also removed a pre-existing stray double backtick
in the zh page while I was in it.
## New tests
`FileUtilsTest` gains four cases, including the H2 regression directly —
content `"earlier line\nabcdefghijklmnopqrstuvwxyz\n"` with `maxBytes=10`
expects `"rstuvwxyz\n"`, which returned `""` before this fix — plus a case
where the break is genuinely mid-window, a platform-charset-independent UTF-8
case, and one pinning `effectiveTailLimit`'s clamp. New `LogContentReaderTest`
covers within-limit and unlimited passthrough and asserts the truncation
notice's shape while checking the body after it is still the file's own tail
starting at a line boundary.
## What I verified, and what I did not
I have no JDK or Maven in this environment, so **CI is the first real
compile** for this change and I am not going to claim otherwise. What I did do
instead of guessing: I ported `readFileTailToStr` / `lineStartOffset` /
`effectiveTailLimit` line-for-line to Python and ran every pre-existing case in
`FileUtilsTest` plus every new one against the port — 28/28 assertions pass,
including a CJK sweep over `maxBytes` from 100 to 400 checking no response ever
starts mid-character, and the single-33-character-line case. That is an
algorithm check, not a substitute for the JVM test run; treat the CI result as
authoritative.
One thing the port did catch: my first draft of the H2 regression test
asserted `"qrstuvwxyz"`. Tracing the byte offsets by hand showed the retained
window is `"rstuvwxyz\n"` — 10 bytes from offset 30 of a 40-byte file — so the
assertion, not the code, was wrong. Worth mentioning because it is the same
off-by-one the fix is about, from the other direction.
@DanielLeens — this head is the one to re-review; the fork's `Build` run for
it has just started.
--
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]
