XComp commented on code in PR #29533:
URL: https://github.com/apache/flink/pull/29533#discussion_r4329914653
##########
flink-runtime/src/test/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManagerTest.java:
##########
@@ -935,6 +939,141 @@ private void testAccessMetricValueDuringItsUnregister(
});
}
+ /**
+ * Tests that {@code runTest} waits for {@code close()} while a
requirements check is still
Review Comment:
I'm not sure whether we should add these tests. They essentially test test
code rather than production code. 🤔 I guess it's good to have created it for
verification purposes but let's not keep it for the merge.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/resourcemanager/slotmanager/FineGrainedSlotManagerTestBase.java:
##########
@@ -234,19 +230,22 @@ protected final void runTest(RunnableWithException
testMethod) throws Exception
resourceEventListenerBuilder.build(),
blockedTaskManagerChecker));
- testMethod.run();
-
- CompletableFuture<Void> closeFuture = new CompletableFuture<>();
- runInMainThread(
- () -> {
- try {
- slotManager.close();
- } catch (Exception e) {
- closeFuture.completeExceptionally(e);
- }
- closeFuture.complete(null);
- });
- FutureUtils.assertNoException(closeFuture);
+ Throwable failure = null;
+ try {
+ testMethod.run();
+ } catch (Throwable t) {
+ failure = t;
+ }
+ // Wait for close(), so afterAll cannot shut the executor down
under a running
+ // requirements check. A failing close() does not hide the test's
own failure.
+ try {
+ FutureUtils.runAsync(slotManager::close,
mainThreadExecutor).get();
Review Comment:
```suggestion
runInMainThreadAndWaitslotManager::close);
```
nit: we could use the newly added utility method
--
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]
