wombatu-kun commented on code in PR #16548:
URL: https://github.com/apache/iceberg/pull/16548#discussion_r3303841651
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestMonitorSource.java:
##########
@@ -221,7 +221,8 @@ void testStateRestore(@TempDir File savepointDir) throws
Exception {
try {
clientWithSavepoint = env.executeAsync("Table Change Source test with
savepoint");
-
assertThat(resultWithSavepoint.poll(Duration.ofSeconds(5L))).isEqualTo(EMPTY_EVENT);
+ // Restoring from a savepoint on a busy cluster may take longer than the
default 5s poll
+
assertThat(resultWithSavepoint.poll(Duration.ofSeconds(30L))).isEqualTo(EMPTY_EVENT);
Review Comment:
The deterministic fix in `closeJobClient` removes the savepoint-completion
race, which was the actual cause of the failure here. I kept the timeout bump
as a separate, defensive measure: restoring a job on a busy CI runner (resubmit
→ schedule → deploy tasks) can occasionally take longer than 5s even when the
savepoint is fully written, so the larger timeout guards against that kind of
CI lag.
It doesn't slow the normal run down, though — `poll(...)` returns as soon as
the expected event arrives, so on an unloaded runner / in the happy path the
extra time is never actually spent; the 30s only caps how long we're willing to
wait before failing. That's why I'd lean towards keeping it as a low-cost
robustness backstop.
That said, I don't feel strongly about it — if you'd rather rely solely on
the deterministic fix and keep the change minimal, just let me know and I'll
revert the timeout part.
--
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]
