Jackie-Jiang commented on code in PR #11628:
URL: https://github.com/apache/pinot/pull/11628#discussion_r1333475161
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignment.java:
##########
@@ -52,6 +52,22 @@ public interface SegmentAssignment {
List<String> assignSegment(String segmentName, Map<String, Map<String,
String>> currentAssignment,
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap);
+ /**
+ * Assign new segment but check if the assignment calculated with
instancePartition is consistent with the one set
+ * in the current idealState. If not, use the one from idealState to stay
consistent with the source of truth.
+ *
+ * @param segmentName Name of the segment to be assigned
+ * @param currentAssignment Current segment assignment of the table (map
from segment name to instance state map)
+ * @param instancePartitionsMap Map from type (OFFLINE|CONSUMING|COMPLETED)
to instance partitions
+ * @param instancesAssigned returns the list of instances to assign the
segment
+ * @return true if consistent or found no existing assignment from
idealState; false otherwise.
+ */
+ default boolean assignSegment(String segmentName, Map<String, Map<String,
String>> currentAssignment,
Review Comment:
I feel this is not a very clean abstraction. We can pass in an optional
`ControllerMetrics` into the `SegmentAssignment`. We don't want to have one new
method whenever we want to emit a metric
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/ZKOperator.java:
##########
@@ -335,7 +335,7 @@ private void processNewSegment(String tableNameWithType,
SegmentMetadata segment
}
try {
- _pinotHelixResourceManager.assignTableSegment(tableNameWithType,
segmentMetadata.getName());
+ _pinotHelixResourceManager.assignTableSegment(tableNameWithType,
segmentMetadata.getName(), _controllerMetrics);
Review Comment:
Very surprised to see we don't have access to controller metrics in the
`PinotHelixResourceManager`. Can you pass it in when calling
`PinotHelixResourceManager.start()`?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
