gabotechs commented on code in PR #3277:
URL: https://github.com/apache/iceberg-rust/pull/3277#discussion_r4310783246
##########
crates/iceberg/src/arrow/reader/mod.rs:
##########
@@ -35,6 +35,8 @@ const DEFAULT_RANGE_FETCH_CONCURRENCY: usize = 10;
const DEFAULT_METADATA_SIZE_HINT: usize = 512 * 1024;
mod file_reader;
+#[cfg(test)]
+mod flow_control_tests;
Review Comment:
This test seems inconsistent with the testing approach in this crate, it
also seems unreasonably long (~500 LOC) for the fairly contained fix in the
`pipeline.rs` file. If it was mostly AI generated, I recommend to do a second
pass trying to contain it a bit more, or even remove it altogether in favor of
a simpler test that provides the same coverage.
Reducing the complexity on that test and the amount of churn will be a good
recipe for getting this PR noticed by the maintainers, as otherwise there are
high chances that this sits in the review backlog for a long time.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -91,9 +91,21 @@ impl ArrowReader {
.try_flatten(),
)
} else {
+ let runtime = self.runtime.clone();
Box::pin(
tasks
- .map_ok(move |task| task_reader.clone().process(task))
+ .map_ok(move |task| {
+ // Open the file on the IO runtime: the flatten below
stops polling
+ // the opens once it holds
`concurrency_limit_data_files` streams, and
+ // a half-read response keeps part of the HTTP/2
connection window,
+ // which stalls every other read on that connection.
Abort on drop,
+ // so a dropped scan stops its opens rather than fetch
for nobody.
+ let opening = runtime
+ .io()
+ .spawn(task_reader.clone().process(task))
+ .abort_on_drop();
+ async move { opening.await? }
Review Comment:
The `FileScanTaskReader` struct already contains a `FileIO` object, that I
would expect to be in charge on reading tasks in the appropriate `tokio`
runtime, rather than manually be handled in each callsite.
If you look at the `FileScanTaskReader::process` method
(https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/arrow/reader/pipeline.rs#L149-L149),
it's not only handling file opening, it's doing many more things, and it might
be a mistake to spawn most of those other things in a runtime that is supposed
to be dedicated to pure IO.
Without being familiar with this code, my intuition tells me that this is
not solved in the appropriate place, and that this PR might need to get more
granular about what pieces of code are spawned in the IO runtime.
--
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]
