[GitHub] [incubator-pinot] snleee commented on a change in pull request #5712: Add startReplaceSegments, endReplaceSegments controller API

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

snleee commented on a change in pull request #5712:
URL: https://github.com/apache/incubator-pinot/pull/5712#discussion_r459142816



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2164,6 +2168,160 @@ public boolean instanceExists(String instanceName) {
     return tableNamesWithType;
   }
 
+  /**
+   * Computes the start segment replace phase
+   *
+   * 1. Generate a batch id
+   * 2. Compute validation on the user inputs
+   * 3. Add the new lineage entry to the segment lineage metadata in the property store
+   *
+   * Update is done with retry logic along with read-modify-write block for achieving atomic update of the lineage
+   * metadata.
+   *
+   * @param tableNameWithType Table name with type
+   * @param segmentsFrom a list of segments to be merged
+   * @param segmentsTo a list of merged segments
+   * @return Batch Id
+   *
+   * @throws InvalidConfigException
+   */
+  public String startReplaceSegments(String tableNameWithType, List<String> segmentsFrom, List<String> segmentsTo) {
+    // Create a batch id
+    String batchId = SegmentLineageUtils.generateLineageEntryId();
+
+    // Check that all the segments from 'segmentsFrom' exist in the table
+    Set<String> segmentsForTable = new HashSet<>(getSegmentsFor(tableNameWithType));
+    Preconditions.checkArgument(segmentsForTable.containsAll(segmentsFrom), String.format(
+        "Not all segments from 'segmentsFrom' are available in the table. (tableName = '%s', segmentsFrom = '%s', "
+            + "segmentsTo = '%s', segmentsFromTable = '%s')", tableNameWithType, segmentsFrom, segmentsTo,
+        segmentsForTable));
+
+    // Check that all the segments from 'segmentTo' does not exist in the table.
+    Preconditions.checkArgument(Collections.disjoint(segmentsForTable, segmentsTo), String.format(
+        "Any segments from 'segmentsTo' should not be available in the table at this point. (tableName = '%s', "
+            + "segmentsFrom = '%s', segmentsTo = '%s', segmentsFromTable = '%s')", tableNameWithType, segmentsFrom,
+        segmentsTo, segmentsForTable));
+

Review comment:
       Good point. It's very important that for any segment, it should appear on `segmentsFrom` at most once because we will have the issue with double-counting otherwise. In order to prevent that, we will do the following:
    
   1. Merge task scheduler will check the segments that are running (started but not finished) not to schedule the segments that are already scheduled.
   2. During the "startReplaceSegments", we reject if the input `segmentFrom` appears any of `segmentFrom` field in the segment lineage.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org