[GitHub] [flink] gaoyunhaii commented on a diff in pull request #20351: [FLINK-28380][runtime] Produce one intermediate dataset for multiple consumer job vertices consuming the same data

[GitBox <gi...@apache.org>]

gaoyunhaii commented on code in PR #20351:
URL: https://github.com/apache/flink/pull/20351#discussion_r931752410


##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamConfig.java:
##########
@@ -92,7 +92,7 @@ public class StreamConfig implements Serializable {
     private static final String TYPE_SERIALIZER_SIDEOUT_PREFIX = "typeSerializer_sideout_";
     private static final String ITERATON_WAIT = "iterationWait";
     private static final String NONCHAINED_OUTPUTS = "nonChainedOutputs";
-    private static final String EDGES_IN_ORDER = "edgesInOrder";
+    private static final String STREAM_OUTPUTS_IN_ORDER = "streamOutputsInOrder";

Review Comment:
   Might be renamed to `VERTEX_NONCHAINED_OUTPUTS` ?
   
   For the long run, I think we might explicit distinguish the vertex-level attributes in the chain head and the normal operator configuration.



##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##########
@@ -676,11 +682,15 @@ private List<StreamEdge> createChain(
                 config.setChainIndex(chainIndex);
                 config.setOperatorName(streamGraph.getStreamNode(currentNodeId).getOperatorName());
 
+                LinkedHashSet<NonChainedOutput> transitiveOutputs = new LinkedHashSet<>();
                 for (StreamEdge edge : transitiveOutEdges) {
-                    connect(startNodeId, edge);
+                    NonChainedOutput output =
+                            opIntermediateOutputs.get(edge.getSourceId()).get(edge);
+                    transitiveOutputs.add(output);
+                    connect(startNodeId, edge, output);
                 }
 
-                config.setOutEdgesInOrder(transitiveOutEdges);
+                config.setStreamOutputsInOrder(new ArrayList<>(transitiveOutputs));

Review Comment:
   Should here also need the de-duplicated outputs?
   
   It looks to me this part should be the list of outputs for the whole vertex, and the non-chained output lists is the per-operator one. Currently it seems at the TM side we first create all the outputs without deduplication, then we connect the operators deduplicated outputs to the chosen ones. This should work, but there seems a bit inconsistent to me



##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##########
@@ -963,6 +975,61 @@ private void setVertexConfig(
         vertexConfigs.put(vertexID, config);
     }
 
+    private List<NonChainedOutput> mayReuseNonChainedOutputs(
+            int vertexId, List<StreamEdge> consumerEdges) {
+        if (consumerEdges.isEmpty()) {
+            return new ArrayList<>();
+        }
+        List<NonChainedOutput> outputs = new ArrayList<>(consumerEdges.size());
+        Map<StreamEdge, NonChainedOutput> outputsConsumedByEdge =
+                opIntermediateOutputs.computeIfAbsent(vertexId, ignored -> new HashMap<>());
+        for (StreamEdge consumerEdge : consumerEdges) {
+            checkState(vertexId == consumerEdge.getSourceId(), "Vertex id must be the same.");
+            int consumerParallelism =
+                    streamGraph.getStreamNode(consumerEdge.getTargetId()).getParallelism();
+            int consumerMaxParallelism =
+                    streamGraph.getStreamNode(consumerEdge.getTargetId()).getMaxParallelism();
+            StreamPartitioner<?> partitioner = consumerEdge.getPartitioner();
+            ResultPartitionType partitionType = getResultPartitionType(consumerEdge);
+            NonChainedOutput output =
+                    new NonChainedOutput(
+                            consumerEdge.supportsUnalignedCheckpoints(),
+                            consumerEdge.getSourceId(),
+                            consumerParallelism,
+                            consumerMaxParallelism,
+                            consumerEdge.getBufferTimeout(),
+                            new IntermediateDataSetID(),
+                            consumerEdge.getOutputTag(),
+                            partitioner,
+                            partitionType);
+
+            if (consumerEdge.getOutputTag() != null || !partitionType.isReconsumable()) {

Review Comment:
   Why can not we do reusing for side-output ?



##########
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/graph/StreamingJobGraphGenerator.java:
##########
@@ -925,8 +935,10 @@ private void setVertexConfig(
 
         config.setStreamOperatorFactory(vertex.getOperatorFactory());
 
-        config.setNumberOfOutputs(nonChainableOutputs.size());
-        config.setNonChainedOutputs(nonChainableOutputs);
+        List<NonChainedOutput> duplicatedOutputs =

Review Comment:
   Might be `deduplicatedOutputs` ?



-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org