[GitHub] [flink] zhuzhurk commented on a diff in pull request #20739: [FLINK-29101] PipelinedRegionSchedulingStrategy benchmark shows performance degradation

[GitBox]

zhuzhurk commented on code in PR #20739:
URL: https://github.com/apache/flink/pull/20739#discussion_r966903922


##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/strategy/PipelinedRegionSchedulingStrategy.java:
##########
@@ -301,10 +335,12 @@ private boolean 
isDownstreamConsumedPartitionGroupSchedulable(
 
     private boolean isDownstreamOfCrossRegionConsumedPartitionSchedulable(
             final ConsumedPartitionGroup consumedPartitionGroup,
-            final SchedulingPipelinedRegion pipelinedRegion) {
+            final SchedulingPipelinedRegion pipelinedRegion,
+            final Set<SchedulingPipelinedRegion> regionToSchedule) {
         if 
(consumedPartitionGroup.getResultPartitionType().canBePipelinedConsumed()) {
             for (IntermediateResultPartitionID partitionId : 
consumedPartitionGroup) {
                 if (isExternalConsumedPartition(partitionId, pipelinedRegion)
+                        && 
!regionToSchedule.contains(getProducerRegion(partitionId))

Review Comment:
   It's better to invoke `getProducerRegion` only once to save some cost



##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/strategy/PipelinedRegionSchedulingStrategy.java:
##########
@@ -61,7 +62,8 @@ public class PipelinedRegionSchedulingStrategy implements 
SchedulingStrategy {
     private final Set<ConsumedPartitionGroup> 
crossRegionConsumedPartitionGroups =

Review Comment:
   consumedPartitionGroupsOfRegion -> producedPartitionGroupsOfRegion
   
   I think this is more accurate and can make the code a bit easier to 
understand. The documentation should be updated accordingly.



##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/strategy/PipelinedRegionSchedulingStrategy.java:
##########
@@ -281,10 +313,12 @@ private boolean areRegionInputsAllConsumable(
     }
 
     private boolean isDownstreamConsumedPartitionGroupSchedulable(
-            final ConsumedPartitionGroup consumedPartitionGroup) {
+            final ConsumedPartitionGroup consumedPartitionGroup,
+            final Set<SchedulingPipelinedRegion> regionToSchedule) {
         if 
(consumedPartitionGroup.getResultPartitionType().canBePipelinedConsumed()) {
             for (IntermediateResultPartitionID partitionId : 
consumedPartitionGroup) {
-                if 
(!scheduledRegions.contains(getProducerRegion(partitionId))) {
+                if (!scheduledRegions.contains(getProducerRegion(partitionId))

Review Comment:
   It's better to invoke `getProducerRegion` only once to save some cost



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