[GitHub] [flink] wsry commented on a diff in pull request #20200: [FLINK-28446][runtime] Expose more information in PartitionDescriptor to support more optimized Shuffle Service

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

wsry commented on code in PR #20200:
URL: https://github.com/apache/flink/pull/20200#discussion_r920828843


##########
flink-runtime/src/main/java/org/apache/flink/runtime/shuffle/PartitionDescriptor.java:
##########
@@ -53,14 +54,22 @@ public class PartitionDescriptor implements Serializable {
     /** Connection index to identify this partition of intermediate result. */
     private final int connectionIndex;
 
+    /** Whether the intermediate result is a broadcast result. */
+    private final boolean isBroadcast;
+
+    /** The distribution pattern of the intermediate result. */
+    private final DistributionPattern distributionPattern;

Review Comment:
   I would suggest to use a boolean type instead of DistributionPattern to avoid exposing a the new internal type which is not public. Currently, this new information is never used in the NettyShuffleMaster, if someone modifies the DistributionPattern, for example, removing some enum fields, the compatibility will be broken and the person who modifies DistributionPattern will never realize that. I think we can use something like this to replace the current field:
   ```private final boolean isAllToAllDistribution;```
   then we only rely on the all to all property and we convert the DistributionPattern to isAllToAllDistribution internally, if someone modifies the code, then he/she must also take care of the internal conversion logic. What do you think?



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