XComp commented on code in PR #23880:
URL: https://github.com/apache/flink/pull/23880#discussion_r1422663705
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/JobGraphWriter.java:
##########
@@ -37,6 +38,18 @@ public interface JobGraphWriter extends
LocallyCleanableResource, GloballyCleana
*/
void putJobGraph(JobGraph jobGraph) throws Exception;
+ /**
+ * Adds the {@link JobGraph} instance and have write operations performed
asynchronously in
+ * ioExecutor of Dispatcher
+ *
+ * @param jobGraph
+ * @param ioExecutor
+ * @return
+ * @throws Exception
+ */
+ CompletableFuture<Void> putJobGraphAsync(JobGraph jobGraph,
Optional<Executor> ioExecutor)
Review Comment:
A few things on the interface change:
1. `Optional<Executor>` is not the usual way we implement async and sync
behavior with a single method. You can rely on the `DirectExecutor` to achieve
the same. There is no need to deal with `Optional`.
2. For cases where you want to have the async and the sync version of a
method being available, the code is usually easier to read if you put the
business logic in the sync method and implement the async method in the
following way:
```java
public void runRandomMethod(Object obj) {
// do something
}
public CompletableFuture<Void> runRandomMethodAsync(Object obj, Executor
executor) {
return FutureUtils.runAsync(() -> runRandomMethod(obj), executor);
}
```
3. I'm wondering whether we should make all `put*` methods in the
`JobGraphWriter` interface asynchronous rather than maintaining a synchonous
`putJobGraph` method along the `putJobGraphAsync` method which is then only
called by `putJobResourceRequirements`. `putJobResourceRequirements` could
block the `Dispatcher` for the very same reason why `putJobGraph` would block.
WDYT?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
