DanielLeens commented on PR #11564:
URL: https://github.com/apache/seatunnel/pull/11564#issuecomment-5363691441
Thanks @SEZ9. On the two asks:
1. **Head SHA** — I can confirm the merge-mechanics half of this:
`headRefOid` for this PR is still `e01712f9` right now, unchanged since it was
pushed on 2026-08-19, and the two files that make up this PR's actual diff
(`ReportMetricsOperation.java`, `ReportMetricsOperationTest.java`) are still
byte-for-byte identical to the head I approved on 08-04. Whether any *further*
push is planned is really a question for @nielifeng to answer directly, since
that's outside what I can attest to from the repo state alone.
2. **Correction to my own last review** — I need to flag something before
this merges. In my 08-19 comment I described the fork `Build` run on `e01712f9`
as still `in_progress` and treated CI-completeness as the only remaining gate.
That run has since finished, and it is now **failing**, not green. I went and
checked the actual failing jobs so this doesn't block on a guess:
- `unit-test (11, windows-latest)`
- `engine-v2-it (8, ubuntu-latest)` — multi-node
`ClusterFaultToleranceIT` restore/master-down scenarios and a `RestApiIT`
heartbeat-timeout pattern
- `all-connectors-it-7 (11, ubuntu-latest)`
- `doris-connector-it (8, ubuntu-latest)` — concretely,
`DorisErrorIT.testDoris:82 expected: <true> but was: <false>`
None of these touch `ReportMetricsOperation`/`ReportMetricsOperationTest`
at all (Doris connector IT and Windows unit-test are obviously unrelated
modules; the `engine-v2-it` failures are fault-tolerance/REST integration
tests, not anything in the metrics-offload call path), and the PR's own diff is
unchanged from the head that passed CI twice before. So this looks like
environment-level flakiness in unrelated modules rather than a regression
introduced here — but it does mean the `Build` check is factually red on
`e01712f9` right now, and `mergeStateStatus` is `BLOCKED`. I'd suggest
re-running just the failed jobs (`gh run rerun <run-id> --failed` on the fork
run) before merging, rather than treating CI as already satisfied.
On the test-stability tracker: agreed a short follow-up issue for the
`shouldKeepRemoteGenericOperationsResponsiveWhileMetricsWaitForOffloadExecutor`
timing-centralization note (mine and yours from 07-28/08-01) is the right way
to keep it from getting lost post-merge — happy to see that filed, no objection
from my side either way on who opens it.
No new source-level issue from me this round; my source conclusion
(APPROVED, one Low-severity non-blocking item) still stands. The only thing
that changed is the CI fact above.
--
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]
