[GitHub] [flink] zhuzhurk commented on a change in pull request #14584: [FLINK-20850][runtime] Removing usage of CoLocationConstraints

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

zhuzhurk commented on a change in pull request #14584:
URL: https://github.com/apache/flink/pull/14584#discussion_r555142323



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java
##########
@@ -331,15 +331,11 @@ public int getNumberOfVertices() {
         return Collections.unmodifiableSet(slotSharingGroups);
     }
 
-    public Set<CoLocationGroupDesc> getCoLocationGroupDescriptors() {
-        // invoke distinct() on CoLocationGroup first to avoid creating
-        // multiple CoLocationGroupDec from one CoLocationGroup
-        final Set<CoLocationGroupDesc> coLocationGroups =
+    public Set<CoLocationGroup> getCoLocationGroup() {

Review comment:
       `getCoLocationGroups` would be better

##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/jobgraph/JobGraphTest.java
##########
@@ -426,7 +426,7 @@ public void testGetSlotSharingGroups() {
     }
 
     @Test
-    public void testGetCoLocationGroupDescriptors() {
+    public void testGetCoLocationGroup() {

Review comment:
       maybe `testGetCoLocationGroups`?

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/CoLocationGroup.java
##########
@@ -19,105 +19,41 @@
 package org.apache.flink.runtime.jobmanager.scheduler;
 
 import org.apache.flink.runtime.jobgraph.JobVertex;
+import org.apache.flink.runtime.jobgraph.JobVertexID;
 import org.apache.flink.util.AbstractID;
-import org.apache.flink.util.Preconditions;
 
-import java.util.ArrayList;
 import java.util.List;
 
 /**
- * A Co-location group is a group of JobVertices, where the <i>i-th</i> subtask of one vertex has to
- * be executed on the same TaskManager as the <i>i-th</i> subtask of all other JobVertices in the
- * same group.
+ * {@code CoLocationGroup} refers to a list of {@link JobVertex} instances, where the <i>i-th</i>
+ * subtask of one vertex has to be executed on the same {@code TaskManager} as the <i>i-th</i>
+ * subtask of all other {@code JobVertex} instances in the same group.
  *
- * <p>The co-location group is used for example to make sure that the i-th subtasks for iteration
- * head and iteration tail are scheduled to the same TaskManager.
+ * <p>The co-location group is used to make sure that the i-th subtasks for iteration head and
+ * iteration tail are scheduled on the same TaskManager.
  */
-public class CoLocationGroup implements java.io.Serializable {
+public interface CoLocationGroup {
 
-    private static final long serialVersionUID = -2605819490401895297L;
-
-    /** The ID that describes the slot co-location-constraint as a group */
-    private final AbstractID id = new AbstractID();
-
-    /** The vertices participating in the co-location group */
-    private final List<JobVertex> vertices = new ArrayList<JobVertex>();
-
-    /** The constraints, which hold the shared slots for the co-located operators */
-    private transient ArrayList<CoLocationConstraint> constraints;
-
-    // --------------------------------------------------------------------------------------------
-
-    public CoLocationGroup() {}
-
-    public CoLocationGroup(JobVertex... vertices) {
-        for (JobVertex v : vertices) {
-            this.vertices.add(v);
-        }
-    }
-
-    // --------------------------------------------------------------------------------------------
-
-    public void addVertex(JobVertex vertex) {
-        Preconditions.checkNotNull(vertex);
-        this.vertices.add(vertex);
-    }
-
-    public List<JobVertex> getVertices() {
-        return vertices;
-    }
-
-    public void mergeInto(CoLocationGroup other) {
-        Preconditions.checkNotNull(other);
-
-        for (JobVertex v : this.vertices) {
-            v.updateCoLocationGroup(other);
-        }
-
-        // move vertex membership
-        other.vertices.addAll(this.vertices);
-        this.vertices.clear();
-    }
-
-    // --------------------------------------------------------------------------------------------
-
-    public CoLocationConstraint getLocationConstraint(int subtask) {
-        ensureConstraints(subtask + 1);
-        return constraints.get(subtask);
-    }
-
-    private void ensureConstraints(int num) {
-        if (constraints == null) {
-            constraints = new ArrayList<CoLocationConstraint>(num);
-        } else {
-            constraints.ensureCapacity(num);
-        }
-
-        if (num > constraints.size()) {
-            constraints.ensureCapacity(num);
-            for (int i = constraints.size(); i < num; i++) {
-                constraints.add(new CoLocationConstraint(this));
-            }
-        }
-    }
+    /**
+     * Returns the unique identifier describing this co-location constraint as a group.
+     *
+     * @return The group's identifier.
+     */
+    AbstractID getId();
 
     /**
-     * Gets the ID that identifies this co-location group.
+     * Returns the IDs of the {@link JobVertex} instances participating in this group.
      *
-     * @return The ID that identifies this co-location group.
+     * @return The group's members represented by their {@link JobVertexID}s.
      */
-    public AbstractID getId() {
-        return id;
-    }
+    List<JobVertexID> getVertexIDs();

Review comment:
       NIT: `getVertexIds()` would be better towards a common naming pattern. There exists some `getXXXID()` but I  think the latest new names are always `getXXXId()`/`getXXXIds()`.




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

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