utafrali commented on code in PR #29116:
URL: https://github.com/apache/flink/pull/29116#discussion_r3943208876
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcherTest.java:
##########
@@ -95,6 +96,13 @@ void setUp() throws Exception {
@AfterEach
void tearDown() throws Exception {
archiveEvents.clear();
+ // Shut the fetcher down before deleting the @TempDir: its executor
threads write into
+ // localArchiveRootPath, and a task still running during cleanup fails
the temp-dir
+ // deletion.
+ if (fetcher != null) {
+ fetcher.close();
Review Comment:
If `fetcher.close()` ever throws, `archiveStorage.close()` below will be
skipped and the storage will leak between tests. Consider wrapping the fetcher
close in a try/finally so the storage close always runs, e.g. `try {
fetcher.close(); } finally { fetcher = null; }` followed by the existing
storage block. `ExecutorUtils.gracefulShutdown` isn't expected to throw today,
but the field-based teardown pattern reads more robustly this way.
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcherTest.java:
##########
@@ -410,8 +409,7 @@ void testCleanUpLazyFetchTaskCancelsRunningFuture() throws
Exception {
archiveStorage, "jobs/" + jobId + "/config");
archiveStorage = blockingStorage;
- HistoryServerArchiveFetcher<?> fetcher =
- createArchiveFetcher(remoteArchiveRootPath, false,
blockingStorage);
+ fetcher = createArchiveFetcher(remoteArchiveRootPath, false,
blockingStorage);
Review Comment:
Worth double-checking: `testCleanUpLazyFetchTaskCancelsRunningFuture` never
counts down `blockingStorage.releaseLatch` before the test returns. The
cancelled future should interrupt the executor thread and unblock it, but if
`BlockingArchiveStorage` uses `CountDownLatch.await()` without an
interrupt-safe path, `fetcher.close()` (via `ExecutorUtils.gracefulShutdown`)
could stall until its timeout during teardown. If you have seen the graceful
shutdown complete quickly on this test in the 62-run loop, then it's fine
as-is; otherwise consider counting the latch down at the end of the test to
guarantee prompt drain.
--
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]
