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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to