shyjsarah commented on code in PR #720:
URL: https://github.com/apache/paimon-rust/pull/720#discussion_r3801452996
##########
crates/paimon/src/vindex/range_reader.rs:
##########
@@ -201,41 +255,74 @@ impl VindexFileReader {
}
fn fetch_exact(&self, range: Range<u64>) -> io::Result<Bytes> {
- let mut results =
self.fetch_range_batch(std::slice::from_ref(&range))?;
- Ok(results.pop().expect("one requested range"))
+ let mut result = None;
+ self.fetch_range_batch(std::slice::from_ref(&range), |_, data| {
+ result = Some(data);
Review Comment:
**[Major] The single-range path releases the response permit before the
payload is actually copied.**
`fetch_range_batch` keeps the response permit only until `consume` returns,
but this closure moves the owned `Bytes` into `result` and returns immediately.
`read_one` performs `copy_from_slice` afterwards (and may retain the same
`Bytes` in the scalar cache), when the response permit has already been
released. Consequently, cloned readers using single-range `pread` can hold more
than the intended global `2C` responses at once; the existing bounded-response
test blocks inside the callback and therefore does not cover this escape path.
Could we either copy into the caller buffer inside the protected callback,
or return an RAII value that binds `Bytes` to its `OwnedSemaphorePermit` until
the copy/cache handoff finishes? A regression test with `C=1` and three clones
doing single-range reads would make the intended bound explicit.
##########
crates/paimon/src/vindex/range_reader.rs:
##########
@@ -257,24 +344,43 @@ impl VindexFileReader {
.returned_bytes
.fetch_add(data.len() as u64, Ordering::Relaxed);
}
- Ok(data)
+ sender
+ .send(Ok((index, data, response_permit)))
+ .await
+ .map_err(|_| io::Error::other("vindex range read
receiver closed"))
}
}))
- .await;
- let _ = sender.send(fetched);
+ .buffer_unordered(response_limit);
Review Comment:
**[Major] The current two-stage admission does not reliably preserve the
cloned-reader fairness asserted by the test.**
Each batch independently polls up to `2C` work items, first acquiring a
response permit and only then entering the I/O semaphore. Ordering can be lost
between those two queues: a clone may obtain/wait for response admission but
not reach the I/O queue before the original batch's refill work does. I
reproduced `cloned_reader_is_not_queued_behind_an_entire_batch` failing once in
the full range-reader suite, while subsequent runs passed. The test hook also
signals before `acquire_owned()` is known to be `Pending`, so it does not
establish the ordering assumed by the assertion.
Please make the test notify only after the response acquire is actually
pending. If cross-reader fairness is a required contract, admission needs to
preserve ordering across the response→I/O transition (for example through a
shared scheduler/queue); otherwise the test and documentation should state the
weaker bounded-delay guarantee rather than assert a specific start order.
--
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]
