XComp commented on code in PR #25027:
URL: https://github.com/apache/flink/pull/25027#discussion_r1680520986
##########
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/DefaultJobManagerRunnerRegistry.java:
##########
@@ -97,6 +106,14 @@ public JobManagerRunner unregister(JobID jobId) {
return this.jobManagerRunners.remove(jobId);
}
+ public void setMainThreadExecutor(ComponentMainThreadExecutor executor) {
+ mainThreadExecutor = executor;
+ }
+
+ public ComponentMainThreadExecutor getMainThreadExecutor() {
+ return mainThreadExecutor;
+ }
+
Review Comment:
Adding setters and maintaining nullable state here hints towards a not
well-designed class structure.
We're trying to solve the issue that the `DefaultJobManagerRunnerRegistry`
requires the main thread executor but still want the main thread verification
being executed in a different class (`OnMainThreadJobManagerRunnerRegistry`).
That doesn't sound right. We might want to merge the two. But this comes with
some problems when initializing the `Dispatcher`.
Can you take a step back and try to reorganize the `Dispatcher`
instantiation logic? One hint here: We created the [base Dispatcher
constructor](https://github.com/apache/flink/blob/583aadf97b2e3ddc87d4d244c5d62823e82513b1/flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/Dispatcher.java#L259)
to improve testability, i.e. to use a testing implementation of
`ResourceCleanerFactory`. So, we might be required to rethink about how we
initialize the `ResourceCleanerFactory` if cannot keep the
`DefaultJobManagerRunnerRegistry` and `OnMainThreadJobManagerRunnerRegistry`
separate.
--
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]
