Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23880: URL: https://github.com/apache/flink/pull/23880#issuecomment-1960628068 @XComp I created a new pr，https://github.com/apache/flink/pull/24366 Please review it when you have time. -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
flinkbot commented on PR #24366: URL: https://github.com/apache/flink/pull/24366#issuecomment-1958766054 ## CI report: * 8ce3e530ab2aef4d06a98f3c0cfa55a49796deee UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
[PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 opened a new pull request, #24366: URL: https://github.com/apache/flink/pull/24366 ## What is the purpose of the change *Currently, submitting a job starts with storing the JobGraph (in HA setups) in the JobGraphStore. This includes writing the file to S3 (or some other remote file system). The job submission is done in the Dispatcher's main thread. If writing the JobGraph is slow, it would block any other operation on the Dispatcher.* ## Brief change log - *The dispatcher put JobGraph asynchronously in ioExecutor* - *The dispatcher write To ExecutionGraphInfoStore asynchronously in ioExecutor* - *The JobGraphWriter interface class adds a putJobGraphAsync method for asynchronous write operations* - *Implementation class DefaultJobGraphStore adds the putJobGraphAsync method for asynchronous write operations* ## Verifying this change This change added tests and can be verified as follows: - *Added the Dispatcher JobSubmission test, use ZooKeeperStateHandleStore as JobGraphStore* - *Added the Dispatcher JobSubmission test, use KubernetesStateHandleStore as JobGraphStore* ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
XComp commented on PR #23880: URL: https://github.com/apache/flink/pull/23880#issuecomment-1956456977 @zhengzhili333 sorry for the delay. But can you rebased the branch and remove any merge commits? That would help with the review -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23880: URL: https://github.com/apache/flink/pull/23880#issuecomment-1857188917 Thank you for your suggestion, I have modified that interface and also corrected the point you mentioned earlier. -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on code in PR #23880:
URL: https://github.com/apache/flink/pull/23880#discussion_r1426783965
##
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 putJobGraphAsync(JobGraph jobGraph,
Optional ioExecutor)
Review Comment:
Yeah, you're right.
--
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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
XComp commented on code in PR #23880:
URL: https://github.com/apache/flink/pull/23880#discussion_r1426546494
##
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 putJobGraphAsync(JobGraph jobGraph,
Optional ioExecutor)
Review Comment:
Can you elaborate a bit more on your claim? Is it because you have to modify
the interface for that change?
If you're concerned about chaning the interface: `JobGraphWriter` is an
internally used interface (i.e. not marked in any way as `@Public` or
`@PublicEvolving`). Additionally, it's not really exposed to users in any way I
could think. In this sense, modifying the interface in order to improve the
overall code base (rather than introducing some workarounds) seems like a
reasonable approach. 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
XComp commented on code in PR #23880:
URL: https://github.com/apache/flink/pull/23880#discussion_r1426546494
##
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 putJobGraphAsync(JobGraph jobGraph,
Optional ioExecutor)
Review Comment:
Can you elaborate a bit more on your claim? Is it because you have to modify
the interface for that change? If you're concerned about chaning the interface:
`JobGraphWriter` is an internally used interface (i.e. not marked in any way as
`@Public` or `@PublicEvolving`). Additionally, it's not really exposed to users
in any way I could think. In this sense, modifying the interface in order to
improve the overall code base (rather than introducing some workarounds) seems
like a reasonable approach. 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on code in PR #23880:
URL: https://github.com/apache/flink/pull/23880#discussion_r1426063623
##
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 putJobGraphAsync(JobGraph jobGraph,
Optional ioExecutor)
Review Comment:
Thank you for your reminder.Changing the interface violates the open/close
principle, but it is not necessary to maintain two interfaces.
--
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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
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 putJobGraphAsync(JobGraph jobGraph,
Optional ioExecutor)
Review Comment:
A few things on the interface change:
1. `Optional` 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 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
flinkbot commented on PR #23880: URL: https://github.com/apache/flink/pull/23880#issuecomment-1842042843 ## CI report: * db9949a710f0dcf6d317053732a0e96b75568944 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
[PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 opened a new pull request, #23880: URL: https://github.com/apache/flink/pull/23880 ## What is the purpose of the change *Currently, submitting a job starts with storing the JobGraph (in HA setups) in the JobGraphStore. This includes writing the file to S3 (or some other remote file system). The job submission is done in the Dispatcher's main thread. If writing the JobGraph is slow, it would block any other operation on the Dispatcher.* ## Brief change log - *The dispatcher put JobGraph asynchronously in ioExecutor* - *The dispatcher write To ExecutionGraphInfoStore asynchronously in ioExecutor* - *The JobGraphWriter interface class adds a putJobGraphAsync method for asynchronous write operations* - *Implementation class DefaultJobGraphStore adds the putJobGraphAsync method for asynchronous write operations* ## Verifying this change This change added tests and can be verified as follows: - *Added the Dispatcher JobSubmission test, use ZooKeeperStateHandleStore as JobGraphStore* - *Added the Dispatcher JobSubmission test, use KubernetesStateHandleStore as JobGraphStore* ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 closed pull request #23866: [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation URL: https://github.com/apache/flink/pull/23866 -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23866: URL: https://github.com/apache/flink/pull/23866#issuecomment-1840210958 @flinkbot run azure -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 closed pull request #23866: [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation URL: https://github.com/apache/flink/pull/23866 -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23866: URL: https://github.com/apache/flink/pull/23866#issuecomment-1839406526 @flinkbot run azure -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23866: URL: https://github.com/apache/flink/pull/23866#issuecomment-1838461664 @flinkbot run azure -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 commented on PR #23866: URL: https://github.com/apache/flink/pull/23866#issuecomment-1838438808 @flinkbot run azure -- 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
Re: [PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
flinkbot commented on PR #23866: URL: https://github.com/apache/flink/pull/23866#issuecomment-1837954195 ## CI report: * 8c0d3fdc4aa77efe4a2736044d3715c96e3dacb8 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
[PR] [FLINK-33500][Runtime] Run storing the JobGraph an asynchronous operation [flink]
zhengzhili333 opened a new pull request, #23866: URL: https://github.com/apache/flink/pull/23866 ## What is the purpose of the change *Currently, submitting a job starts with storing the JobGraph (in HA setups) in the JobGraphStore. This includes writing the file to S3 (or some other remote file system). The job submission is done in the Dispatcher's main thread. If writing the JobGraph is slow, it would block any other operation on the Dispatcher.* ## Brief change log - *The ZooKeeperStateHandleStore create path in ZooKeeper and locks it then write asynchronously state in Executor* - *The KubernetesStateHandleStore stores key in ConfigMap and write asynchronously state in Executor* - *The dispatcher put JobGraph asynchronously in ioExecutor * - *The dispatcher write To ExecutionGraphInfoStore asynchronously in ioExecutor * ## Verifying this change This change added tests and can be verified as follows: - *Added the Dispatcher JobSubmission test, use ZooKeeperStateHandleStore as JobGraphStore* - *Added the Dispatcher JobSubmission test, use KubernetesStateHandleStore as JobGraphStore* ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) -- 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
