slfan1989 commented on PR #559: URL: https://github.com/apache/fluss-rust/pull/559#issuecomment-4531698722
> @slfan1989 Ty for the PR, great addition, LGTM overall, couple of minor comments 👍 > > Let's move this to a separate file for testing RecordBatchLogReader feature and then add some additional scenarios: *until_offsets_with_empty_range (stop == start -> no batches) > > * until_offsets_past_end_of_log (graceful finish past actual end) > * until_offsets_multi_bucket (multiple buckets in the HashMap) > > also `extract_ids_from_batches` overlaps with `extract_ids` in test_project test, mb we wish to factor this out to utils and reuse. @fresh-borzoni Thank you for the review and suggestions! Addressed most of the comments: - Moved the `RecordBatchLogReader` integration coverage into a dedicated test file: `record_batch_log_reader.rs`. - Added `until_offsets_with_empty_range`. - Added `until_offsets_multi_bucket`. - Moved `extract_ids_from_batches` into `integration/utils.rs` and reused it from both `log_table.rs` and `record_batch_log_reader.rs`. I left `until_offsets_past_end_of_log` out for now because it requires a behavior/API decision. The current `new_until_offsets` implementation waits until the requested stop offset is observed. If the stop offset is beyond the current log end, it keeps polling for future records rather than finishing. Supporting graceful completion would require snapshot-style semantics by querying latest offsets at reader creation, which needs `FlussAdmin` and async work. I think that should be handled separately from this test-only PR. Thanks again! -- 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]
