dbtsai commented on PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#issuecomment-5009057876
## Discussion: `AnalyticsCoreUtil.readVectored` over-counts failed / short
ranges
Splitting this out from the inline threads since it is a deliberate design
tradeoff, not a bug to silently fix.
The GCS analytics-core vectored path now counts `range.length()`
**synchronously on the caller thread**, before delegating to
`stream.readVectored(...)`:
```java
for (FileRange range : ranges) {
readBytes.increment(range.length());
readOperations.increment();
}
stream.readVectored(objectRanges, allocate);
```
This was the fix for @viirya's catch that counting in the range-future
completion callbacks attributes bytes to an analytics-core background thread —
under `HadoopMetricsContext`, `READ_BYTES` maps to
`FileSystem.Statistics.incrementBytesRead`, which accumulates per-thread, and
Spark reads task input bytes from the *task* thread. Background-thread counting
is effectively a no-op for Spark task metrics, which is the exact scenario
#17208 is about. So correct-thread attribution has to win.
The known imprecision that buys us that:
- **Short read near EOF** — we count the requested length, not the delivered
bytes (bounded over-count).
- **Failed range** — if `stream.readVectored(...)` throws, the bytes were
already counted (the delegate throws after the increment loop).
My read: for a task-input *metric*, magnitude-correct-and-visible beats
precise-but-invisible, and both errors are bounded. But I want to surface it
explicitly rather than bury it in a code comment.
Options if the over-count is a concern:
1. **Keep as-is** (current) — synchronous, requested-length, documented
imprecision.
2. **Count `range.length()` before delegating, but subtract on the failure
path** — recovers the failed-range case; short-read imprecision remains
(delivered size isn't known synchronously).
3. **Callback-based counting** — precise per delivered bytes, but
wrong-thread attribution → doesn't reach Spark. Rejected for that reason.
Preference? cc @szehon-ho @JoshRosen @viirya — I'm inclined to keep option 1
but happy to move to option 2 if we want to tighten the failed-range case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
