SEZ9 commented on PR #12316: URL: https://github.com/apache/seatunnel/pull/12316#issuecomment-6459401910
Thanks @davidzollo for the explanation of d6d2e8ab1007b1ce2ab67770d3fa5cab1d172084. I have not verified that diff myself, so I am treating the import-only description as stated intent for now; a pointer to the hunk would help me confirm it. Status of the earlier review points against this head: - **PR12316-F1 / F2 (timer FlushSignal path and reader-side wait):** the PR author's own self-assessment in this thread (explicitly not a self-approval, static reading only) describes `onTimerTick` as wrapping its `synchronized(checkpointLock)` body with `injectorArriving()` / `injectorFinished()` the same way `triggerBarrier` does, which would also make `awaitInjectors()` yield to a pending timer flush. If that is accurate at the current head, both findings are resolved. Since an import-only fix would not have changed this on its own, please confirm with a short pointer to the hunk so I can close them. - **PR12316-F3 (`injectorAcquiresLockWithinOnePollCycle`):** if the assertions are unchanged, the `completedPolls` sample is still taken before `injectorArriving()`. Please either move the sample after registration or make the bound tolerant of one extra poll; a scheduling-dependent `pollsWaited <= 1` is likely to flake eventually. - **PR12316-F4 (`Thread.sleep(1)` loop in `awaitInjectors`):** still non-blocking. If you prefer to keep the polling loop for simplicity, a one-line comment noting that resumption latency is bounded by timer granularity is sufficient. - **PR12316-F5 (class javadoc):** please reword "an injector never waits for the reader" so it reads as an ordering/cooperation guarantee rather than a latency guarantee, and add a sentence stating whether the timer flush contender is covered by the handoff. On CI, the latest follow-up on this thread reports the Build check on head d6d2e8ab1007 as completed and green; I have not checked it independently, so I am taking that at face value. Once F3 and F5 are addressed and F1/F2 are confirmed, I have no further concerns from my side. <!-- streview-comment:2085 --> -- 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]
