924060929 commented on PR #68007: URL: https://github.com/apache/doris/pull/68007#issuecomment-6056971961
Re-reviewed current head `a4dd3f02da0ab4414ee9257ea0d7936bc018531f`. ONCE should advance only committed batches and reach FINISHED after exhaustion. The terminal-state locking, cloud persisted-offset synchronization, and recovered empty-tail handling look addressed. However, the following existing findings remain reproducible on this head, so I recommend addressing them before approval. Validation: JDK 17, using `run-fe-ut.sh`. All 25 existing tests across the five relevant test classes passed. Five additional temporary boundary tests failed with assertion failures (no test execution errors): - **[P2] Unicode cursor ordering** ([existing thread](https://github.com/apache/doris/pull/68007#discussion_r4022674298)): after committing a key containing U+E000 and successfully listing its U+1F600 successor, `hasReachedEnd()` is false but `hasMoreDataToConsume()` is also false. With one file per batch, the successor is never scheduled. S3's UTF-8 byte order and Java's UTF-16 `String.compareTo()` disagree here. Prefer explicit readiness from the listing result, or use the filesystem's ordering consistently. This comparison predates the PR, but ONCE still depends on it. - **[P2] Initially empty source** ([existing thread](https://github.com/apache/doris/pull/68007#discussion_r4014174169)): a successful empty metadata listing leaves `hasReachedEnd()` false and readiness true. The existing provider test also confirms `getNextOffset()` throws for this case. Following the actual scheduler/task path, an empty source therefore consumes execution retries and auto-resume attempts. Handle it as exhaustion or a no-data wait, rather than task failure. - **[P2] Non-S3 source validation** ([existing thread](https://github.com/apache/doris/pull/68007#discussion_r4014174175)): ONCE properties pass validation, and creating a `cdc_stream` provider succeeds without rejecting the S3-only property. The provider ignores this mode. Validate applicability after resolving the source type in CREATE and ALTER; the FROM-source JDBC path needs the same check. - **[P2] Local final-commit success time** ([existing thread](https://github.com/apache/doris/pull/68007#discussion_r4022674301)): replaying a terminal S3 transaction with `commitTime=12345` restores exhaustion but leaves `lastTaskSuccessTime=0`. Recovery can then finish permanently with a blank/stale success time. Restore it idempotently from the persisted transaction commit time. The separate cloud count/time finding also remains visible in the code, but was not exercised by these additional tests. - **[P3] Terminal EndOffset display** ([existing thread](https://github.com/apache/doris/pull/68007#discussion_r4022377263)): restoring `{"endFile":"data/b.csv","lastBatch":true}` reports exhaustion, but `getShowMaxOffset()` returns null. Reconstruct the terminal display from the committed cursor. The added tests used mocked S3 listings and transaction replay; this was not a live S3 regression run. They have been removed from the review worktree, and no production code was changed. -- 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]
