SEZ9 commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6258933613
Thanks for keeping the head at `7948e2640` while @nzw921rx's re-review is pending — that makes it easy to check the earlier points against a single commit. I'd like to close the previous findings on diff evidence from this head rather than on thread comments, so it would help if you could point to where in `7948e2640` each of these is handled: - **F4 (docs)**: the benchmark-docs notes removed in `9a6077c58` — I'll confirm on the current diff that the benchmark docs pages are no longer touched. - **F2 / F5 (`JobHistoryService`)**: how the finished-metrics merge is made safe for concurrent callers on the same job (lock keyed per job ID rather than per instance), and the current Javadoc wording for that method. - **F1 / F3 (benchmark durability sample)**: how a failed write-through append on a non-final iteration is surfaced, and whether the teardown check covers the full last batch (e.g. a single `loadAll`) rather than only the last key. If that trade-off is intentional, please say so and I'll review it as such. - **F6 (test)**: whether the test now asserts on the observable merge result instead of `never().computeIfAbsent`, and whether the null-metrics and TTL-preserving paths are covered. Once I have those pointers I'll do my pass on `7948e2640`. No need to rebase before then — if you do push anything, please call out the new head here so the review lands on the right commit. <!-- streview-comment:1871 --> -- 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]
