xintongsong commented on a change in pull request #11248: [FLINK-16299] Release
containers recovered from previous attempt in w…
URL: https://github.com/apache/flink/pull/11248#discussion_r386071286
##########
File path:
flink-yarn/src/test/java/org/apache/flink/yarn/YarnResourceManagerTest.java
##########
@@ -222,6 +222,10 @@ MainThreadExecutor getMainThreadExecutorForTesting() {
return super.getMainThreadExecutor();
}
+ void setTestingNMClientAsync(TestingNMClientAsync
testingNMClientAsync) {
+ this.testingNMClientAsync = testingNMClientAsync;
+ }
Review comment:
Regarding your concern:
- In most cases, people can simply set the functions before starting the RM,
i.e., before `runTest`.
- In cases changing functions during the test, one can easily make sure the
change of the function happens after calls to the previous function with
`CompletableFuture`. Similar to what we do with
`rpcService.(re)setRpcGatewayFutureFunction` in
`ResourceManagerTaskExecutorTest#testDelayedRegisterTaskExecutor`.
- Even if someone messed it up, it should only affect certain test case,
which is easy to locate. No production code or other test cases will be
affected.
I agree that adding a builder class for `Context` could also be an option.
However, I think overwriting the `TestingNMClientAsync` functions might be a
better solution, for the following advantages compared to adding builder for
`Context`:
- It narrows down the scope that we need to customize in a test case. We
need only provide a function that defines the NM client behavior (exactly what
actually needs to be customized for the test case), instead of the entire
`TestingNMClientAsync`.
- It provides the flexibility of changing NM client behavior during the
test, which might not be needed ATM but might be in the future.
- Currently we define the test cases in double brace initialization of the
`Context` class, providing very good readability. I believe the double brace
initialization is required to directly follow the constructor, which means
adding a builder to `Context` would prevent the usage of it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
