davidzollo commented on PR #11856: URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5549171990
Pushed `9e433426e49` addressing the open items from the last full re-review (the two Medium findings plus the carried-over Low ones), and closing out the CI question on the previous head. **Issue 1 (Medium) — duplicated offline-message template / TTL rule.** `CoordinatorService.buildMemberRemovedOfflineMessage` and `isGracefulMemberRemovalMarkerValid` are now `public static` and are the single source of truth; `PhysicalVertex.recordMemberRemovedFailure` delegates to both instead of re-deriving the string and the `Math.abs(...) <= TTL` check. No behavior change — both copies computed the same thing — but the master-failover restore path can no longer drift from the membership-callback path. `PhysicalVertexTest.shouldAgreeWithCoordinatorRulesAtMarkerTtlBoundaries` pins the two consumers together at the TTL boundaries (absent / fresh / exactly-at-TTL / one-past-TTL on both sides), so an edit to either side alone now fails a test. **Issue 2 (Medium) — restore-path integration had no test.** `checkTaskGroupIsExecuting` is now package-visible (`@VisibleForTesting`, no logic change) and three new tests drive it against mocked `NodeEngine` / `HazelcastInstance` / `ClusterService` / `IMap`, with the task recorded as `RUNNING`, its owned slot pointing at a worker that is no longer a cluster member: - fresh marker present → the vertex reads `engine_gracefulMemberRemoval` keyed by the worker address, records the shared offline message classified graceful, returns `false`, and never touches the operation service; - no marker → same message, classified unproven (ERROR path, identical to `dev`); - marker read throws → exception swallowed, still classified unproven (fails safe toward ERROR). The map-name and key-type wiring the review flagged as unverifiable is now exercised, not just the pure helper. **Issue 3 (Low) — single-line `/** ... */` Javadocs.** This turned out to be formatter-mandated rather than a slip: the repo runs google-java-format 1.7 (AOSP) through Spotless with no Javadoc opt-out, and GJF collapses any Javadoc whose text fits within the line limit back to the single-line form — I hand-wrapped all of them and `spotless:apply` immediately re-collapsed the short ones, so the `Code style` job would reject a hand-wrapped version. `docs/en/developer/coding-guide.md` has no guidance on Javadoc form either. What I did instead: where a one-liner was genuinely under-explaining its constraint (why the marker is read without clearing, why a stale marker must be cleared, why only the graceful classification downgrades the level, why the message and flag are held together), the comment now says so and naturally spans multiple lines; the remaining short ones stay in the form the formatter produces. **Issue 4 (Low) — `.exceptionally()` branch untested.** `SeaTunnelServerShutdownTest.shouldAbsorbFailedAsyncMarkerClearWithoutBlockingStartup` returns a pending future from `removeAsync`, asserts a completion handler is chained before completion (i.e. the clear is not fire-and-forget), then fails the future and asserts nothing escapes the lifecycle callback and no blocking `remove` fallback occurs. The warning text itself is not asserted (static Hazelcast logger). **Issue 5 (Low, informational)** — intentionally unchanged: it fails safe toward ERROR and the review marked it optional. **CI on the previous head (`c646f4cd8`):** the Build workflow finished 78 pass / 10 skipped / 1 fail. The one failure was `PostgresCDCIT.testPostgresCdcSnapshotOnlyAndCommittedOffsetStartupModes` (`ConditionTimeoutException`, expected 1 row got 0 after committed-offset restore) in `connector-cdc-postgres-e2e` — a module this Zeta-only diff does not touch. That exact symptom is the open upstream Postgres-CDC bug #11847 (fix in flight in #11864), and the suite has a history of timing flakes (#11644, #11202). Rather than rerun that job in isolation I let this push start a fresh full run; if only that flake recurs I'll rerun it at job granularity. Local verification was limited to module-scoped `spotless:apply`; compile and tests run on GitHub CI against this head. @SEZ9 with this the Medium findings from the latest re-review are closed on the current head — whenever you have time, a fresh look would be appreciated. -- 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]
