shyjsarah commented on PR #782: URL: https://github.com/apache/paimon-rust/pull/782#issuecomment-5507692582
Thanks for adding `$consumers`. The filter pushdown and Java-compatible JSON handling look good overall. I found two minor compatibility/correctness issues: 1. **The overwrite retry window is shorter than Java's** (`crates/paimon/src/table/consumer_manager.rs:27-28,66-84`). Java `release-1.3` retries mismatched consumer JSON 10 times with a 1-second delay, while this implementation uses 200 ms, exhausting the retries after about 1.8 seconds. I reproduced a case where the file becomes valid after 2.1 seconds: Rust returns `DataInvalid`, while Java would still be retrying. Could we align the interval/window with Java and add a delayed-overwrite test? 2. **A POSIX consumer ID containing a backslash is silently omitted by `list_all`** (`crates/paimon/src/table/consumer_manager.rs:103-113`). The listed filename preserves `\\`, but the local `file://` path conversion normalizes it to `/` when `get(id)` reconstructs the path, so a file such as `consumer-id\\part` is listed and then read from a different path. Reading the listed `FileStatus.path` directly, or consistently validating such IDs, would avoid this. Non-blocking: consumer files are currently read serially. Bounded concurrency may be worth considering for object-store tables with many consumers. I also ran the PR's targeted tests and clippy commands locally; they all passed. -- 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]
