[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-12 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r993681403


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -298,6 +303,27 @@ private JobGraph createJobGraph() {
 return jobGraph;
 }
 
+private void waitForSerializationFuturesAndUpdateJobVertices()
+throws ExecutionException, InterruptedException {
+for (JobVertex jobVertex : jobGraph.getVertices()) {
+final 
List>>
+futuresForJobVertex = 
coordinatorSerializationFutures.remove(jobVertex.getID());
+if (futuresForJobVertex == null) {
+LOG.warn(
+"No OperatorCoordinator creator serialized for 
JobVertex {}.",
+jobVertex.getID());
+} else {
+FutureUtils.combineAll(futuresForJobVertex)
+.get()
+.forEach(jobVertex::addOperatorCoordinator);
+}
+}
+Preconditions.checkState(
+coordinatorSerializationFutures.isEmpty(),
+"There are still serialization futures not processed for 
JobVertex instances: {}",

Review Comment:
   I rephrased the condition that should never occur... Were you suggesting 
that we should still wait for the concurrent operations to be completed? 
:thinking: I didn't implement that because this specific state would mean that 
there's some bug somewhere else. The Precondition should trigger the 
`IllegalStateException` causing the JobGraph creation to fail.



-- 
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

[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-12 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r993681403


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -298,6 +303,27 @@ private JobGraph createJobGraph() {
 return jobGraph;
 }
 
+private void waitForSerializationFuturesAndUpdateJobVertices()
+throws ExecutionException, InterruptedException {
+for (JobVertex jobVertex : jobGraph.getVertices()) {
+final 
List>>
+futuresForJobVertex = 
coordinatorSerializationFutures.remove(jobVertex.getID());
+if (futuresForJobVertex == null) {
+LOG.warn(
+"No OperatorCoordinator creator serialized for 
JobVertex {}.",
+jobVertex.getID());
+} else {
+FutureUtils.combineAll(futuresForJobVertex)
+.get()
+.forEach(jobVertex::addOperatorCoordinator);
+}
+}
+Preconditions.checkState(
+coordinatorSerializationFutures.isEmpty(),
+"There are still serialization futures not processed for 
JobVertex instances: {}",

Review Comment:
   I rephrased the condition that should never occur... Were you suggesting 
that we should still wait for the concurrent operations to be completed? 
:thinking: I didn't implement that because this specific state would mean that 
there's some bug somewhere else. The Precondition should trigger the 
`IllegalStateException` causing the cluster to fail fatally.



-- 
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

[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-12 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r993677013


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -298,6 +303,27 @@ private JobGraph createJobGraph() {
 return jobGraph;
 }
 
+private void waitForSerializationFuturesAndUpdateJobVertices()
+throws ExecutionException, InterruptedException {
+for (JobVertex jobVertex : jobGraph.getVertices()) {
+final 
List>>
+futuresForJobVertex = 
coordinatorSerializationFutures.remove(jobVertex.getID());
+if (futuresForJobVertex == null) {
+LOG.warn(
+"No OperatorCoordinator creator serialized for 
JobVertex {}.",
+jobVertex.getID());

Review Comment:
   :+1: Initially, I assumed that we would have to have a serialized 
`OperatorCoordinator` for each of the vertices. But going through the 
`JobGraph` creation, I realized that this is not the case. But in any case, 
using Precondition instead of logging a warning would have been the better 
option.
   
   Anyway, I updated the code and changed the for loop to iterate over the map 
of futures, instead.



-- 
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

[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-11 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r992679151


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -841,6 +842,12 @@ private StreamConfig createJobVertex(Integer streamNodeId, 
OperatorChainInfo cha
 },
 serializationExecutor));
 }
+coordinatorSerializationFutures.add(
+FutureUtils.combineAll(serializationFutures)
+.thenAccept(
+serializedCoordinators ->
+serializedCoordinators.forEach(
+
jobVertex::addOperatorCoordinator)));

Review Comment:
   Ok, (as always :innocent: ) forget my previous statement. I misunderstood 
your concern initially. I pushed another proposal in 
383bb9380f82a97cb8ae3d211add64ffbaa5f023



-- 
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

[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-11 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r992414646


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -841,6 +842,12 @@ private StreamConfig createJobVertex(Integer streamNodeId, 
OperatorChainInfo cha
 },
 serializationExecutor));
 }
+coordinatorSerializationFutures.add(
+FutureUtils.combineAll(serializationFutures)
+.thenAccept(
+serializedCoordinators ->
+serializedCoordinators.forEach(
+
jobVertex::addOperatorCoordinator)));

Review Comment:
   hm, yeah that's what I had in mind. But if we want to do it properly, we'd 
have to pass the `Collection>` as a 
constructor parameter rather than using the `JobVertex#addOperatorCoordinator` 
(which we would remove then). Am I following here correctly?



-- 
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

[GitHub] [flink] XComp commented on a diff in pull request #21019: [FLINK-29576][runtime] Adds concurrency support to JobVertex#addOperatorCoordinator

2022-10-11 Thread GitBox 


XComp commented on code in PR #21019:
URL: https://github.com/apache/flink/pull/21019#discussion_r992375638


##
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##
@@ -841,6 +842,12 @@ private StreamConfig createJobVertex(Integer streamNodeId, 
OperatorChainInfo cha
 },
 serializationExecutor));
 }
+coordinatorSerializationFutures.add(
+FutureUtils.combineAll(serializationFutures)
+.thenAccept(
+serializedCoordinators ->
+serializedCoordinators.forEach(
+
jobVertex::addOperatorCoordinator)));

Review Comment:
   ok, I didn't think that far. :+1: 



-- 
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