JingsongLi commented on PR #9133:
URL: https://github.com/apache/paimon/pull/9133#issuecomment-5235298229
I reviewed the final lane-based diff specifically for scope and unnecessary
changes. The lane ownership model itself looks reasonable, but I think the PR
can be narrowed further.
### Changes that can be removed directly
1. **Remove the remaining `supports_concurrent_pread` declarations and test
attributes.**
The final lane implementation never reads this capability. It was used by
the earlier shared-stream pool, but that consumer was removed in commit
`730db9a`. The declarations in HDFS/Jindo, the Jindo assertion, and the
fake-stream attributes in `blob_test.py` are now dead scaffolding.
2. **Restore the original global `supports_pread` / `pread` helpers.**
The new `_fileno()` helper changes `pread()` from preferring `read_at` to
preferring `fileno() + os.pread`. Stream reuse does not require this change,
and these helpers are also used by the btree, bitmap, vindex, caching, and
mosaic readers. This therefore changes behavior outside the BLOB path and
should not be bundled into this PR.
3. **Remove two redundant tests.**
- `test_fetch_bodies_reuses_bounded_streams_for_same_uri` duplicates the
same-path reuse/cap/close assertions already covered by
`test_same_path_uses_bounded_exclusive_lanes`, while locking the implementation
to exactly 16 opens/closes.
- `test_reader_adapter_read_at_is_concurrent` exercises generic lane
concurrency rather than HDFS adapter behavior and overlaps the lane concurrency
tests in `blob_test.py`.
### Changes that should preferably be split into backend-specific PRs
- **HDFS `read_at/read_range`:** exclusive lanes can reuse and concurrently
execute the existing `seek + read` implementation on separate handles. Native
`read_range` is an additional positional-read optimization, not a dependency of
stream reuse.
- **Jindo lazy/direct adapter:** the existing `PythonFile` can already be
retained and reused by one lane. Bypassing it to expose native `pread` is a
separate optimization. It also changes the generic
`PyArrowFileIO.new_input_stream` return type for every Jindo caller. In
particular, the new `JindoInputFile` does not implement `readinto`, while
`OffsetInputStream.readinto` delegates directly to the wrapped stream, so this
widens the compatibility surface beyond this feature.
Moving the HDFS/Jindo work out would remove about 210 touched lines from the
core PR and leave it focused on lane scheduling and stream lifecycle.
### Needs justification
`_MAX_RANGE_LANES_PER_PATH = 16` is a resource policy rather than a
correctness requirement. The requested `parallelism` already bounds the total
number of active streams, while the hard-coded cap prevents a single path from
using requested parallelism above 16. Please either provide
benchmark/resource-limit evidence for 16, make it configurable, or rely on the
existing parallelism bound.
I would keep the path-aware lane assignment, exclusive cursor access for
unknown-length ranges, closing a failed lane before fallback, global stream
bounding across paths, and the close-error/read-error handling. Those are
meaningful correctness and resource-lifecycle parts of the final design.
--
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]
