[GitHub] [pinot] snleee commented on a diff in pull request #10679: Allow custom lineage update and gc

["snleee (via GitHub)" <gi...@apache.org>]

snleee commented on code in PR #10679:
URL: https://github.com/apache/pinot/pull/10679#discussion_r1182153938


##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java:
##########
@@ -49,15 +49,22 @@ public class SegmentLineage {
 
   private final String _tableNameWithType;
   private final Map<String, LineageEntry> _lineageEntries;
+  private final Map<String, String> _simpleFields;

Review Comment:
   Instead of having `simpleFields/mapFields`, I think that it's better to have a single `Map<String, String> _customMap`?



##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/EndReplaceSegmentsRequest.java:
##########
@@ -21,21 +21,31 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
- * segmentsTo: The new segments that actually get created. Sometimes not all segments that are passed into
+ * 1. segmentsTo: The new segments that actually get created. Sometimes not all segments that are passed into

Review Comment:
   Can we add the title saying that this class is defining `EndReplaceSegmentsRequest`?
   
   e.g. `Request object for startReplaceSegments API.`



##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/RevertReplaceSegmentsRequest.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+
+/**
+ * customMap : custom map.

Review Comment:
   Can we add the documentation that indicates this class is for `RevertReplaceSegmentsRequest`
   
   e.g. `Request object for startReplaceSegments API.`



##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -258,6 +258,7 @@ private static long getRandomInitialDelayInSeconds() {
   public static final String ACCESS_CONTROL_FACTORY_CLASS = "controller.admin.access.control.factory.class";
   public static final String ACCESS_CONTROL_USERNAME = "access.control.init.username";
   public static final String ACCESS_CONTROL_PASSWORD = "access.control.init.password";
+  public static final String LINEAGE_MANAGER_CLASS = "controller.lineage.manager";

Review Comment:
   `controller.lineage.manager.class`



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -241,12 +248,25 @@ public String load(String instanceId) {
     for (int i = 0; i < _tableUpdaterLocks.length; i++) {
       _tableUpdaterLocks[i] = new Object();
     }
+    _lineageManager = lineageManager;
   }
 
   public PinotHelixResourceManager(ControllerConf controllerConf) {
     this(controllerConf.getZkStr(), controllerConf.getHelixClusterName(), controllerConf.getDataDir(),
         controllerConf.tenantIsolationEnabled(), controllerConf.getEnableBatchMessageMode(),
-        controllerConf.getHLCTablesAllowed(), controllerConf.getDeletedSegmentsRetentionInDays());
+        controllerConf.getHLCTablesAllowed(), controllerConf.getDeletedSegmentsRetentionInDays(),
+        getLineageManager(controllerConf));
+  }
+
+  public static LineageManager getLineageManager(ControllerConf controllerConf) {

Review Comment:
   Let's consider to put this logic to `LineageManagerFactory`



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/lineage/LineageManager.java:
##########
@@ -0,0 +1,73 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.api.lineage;

Review Comment:
   Is there a better place to put this? 
   
   RetentionManager is in `org.apache.pinot.controller.helix.core` and LineageManager is mainly utilized under `PinotHelixResourceManager`, which is also under `org.apache.pinot.controller.helix.core`.
   
   Can you double check if this is a better candidate?
   
   



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -185,6 +187,10 @@ public class PinotHelixResourceManager {
   private static final int DEFAULT_TABLE_UPDATER_LOCKERS_SIZE = 100;
   private static final String API_REQUEST_ID_PREFIX = "api-";
 
+  private static enum ReplaceSegmentType {

Review Comment:
   Maybe the better naming is `LineageUpdateType`?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/lineage/DefaultLineageManager.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.api.lineage;
+
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.apache.pinot.common.lineage.LineageEntry;
+import org.apache.pinot.common.lineage.LineageEntryState;
+import org.apache.pinot.common.lineage.SegmentLineage;
+import org.apache.pinot.controller.ControllerConf;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.utils.IngestionConfigUtils;
+
+
+public class DefaultLineageManager implements LineageManager {
+  private static final long REPLACED_SEGMENTS_RETENTION_IN_MILLIS = TimeUnit.DAYS.toMillis(1L); // 1 day
+  private static final long LINEAGE_ENTRY_CLEANUP_RETENTION_IN_MILLIS = TimeUnit.DAYS.toMillis(1L); // 1 day
+
+  protected ControllerConf _controllerConf;
+
+  public DefaultLineageManager(ControllerConf controllerConf) {
+    _controllerConf = controllerConf;
+  }
+
+  @Override
+  public void updateLineageForStartReplaceSegments(TableConfig tableConfig, String lineageEntryId,
+      Map<String, String> customMap, SegmentLineage lineage) {
+  }
+
+  @Override
+  public void updateLineageForEndReplaceSegments(TableConfig tableConfig, String lineageEntryId,
+      Map<String, String> customMap, SegmentLineage lineage) {
+  }
+
+  @Override
+  public void updateLineageForRevertReplaceSegments(TableConfig tableConfig, String lineageEntryId,
+      Map<String, String> customMap, SegmentLineage lineage) {
+  }
+
+  @Override
+  public void updateLineageForRetention(TableConfig tableConfig, SegmentLineage lineage, List<String> allSegments,

Review Comment:
   Can I assume that we are simply moving the retention manager's logic? Or, are we adding any extra logic here?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3723,13 +3764,26 @@ private boolean writeLineageEntryWithTightLoop(String tableNameWithType, String
       if (!currentLineageEntry.equals(lineageEntryToMatch)) {
         String errorMsg = String.format(
             "Aborting the to update lineage entry since we find that the entry has been modified for table %s, "
-                + "entry id: %s", tableNameWithType, lineageEntryId);
+                + "entry id: %s", tableConfig, lineageEntryId);
         LOGGER.error(errorMsg);
         throw new RuntimeException(errorMsg);
       }
 
       // Update lineage entry
       segmentLineageToUpdate.updateLineageEntry(lineageEntryId, lineageEntryToUpdate);
+      switch (replaceSegmentType) {
+        case START:
+          _lineageManager.updateLineageForStartReplaceSegments(null, lineageEntryId, customMap, segmentLineageToUpdate);

Review Comment:
   Can we pass `tableConfig` since it's available? The implementation of `LineageManager` can choose whether to use it or not.



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3723,13 +3764,26 @@ private boolean writeLineageEntryWithTightLoop(String tableNameWithType, String
       if (!currentLineageEntry.equals(lineageEntryToMatch)) {
         String errorMsg = String.format(
             "Aborting the to update lineage entry since we find that the entry has been modified for table %s, "
-                + "entry id: %s", tableNameWithType, lineageEntryId);
+                + "entry id: %s", tableConfig, lineageEntryId);
         LOGGER.error(errorMsg);
         throw new RuntimeException(errorMsg);
       }
 
       // Update lineage entry
       segmentLineageToUpdate.updateLineageEntry(lineageEntryId, lineageEntryToUpdate);
+      switch (replaceSegmentType) {
+        case START:

Review Comment:
   Should we throw exception in this case since it's not the valid path? Or, this is fine?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3723,13 +3764,26 @@ private boolean writeLineageEntryWithTightLoop(String tableNameWithType, String
       if (!currentLineageEntry.equals(lineageEntryToMatch)) {
         String errorMsg = String.format(
             "Aborting the to update lineage entry since we find that the entry has been modified for table %s, "
-                + "entry id: %s", tableNameWithType, lineageEntryId);
+                + "entry id: %s", tableConfig, lineageEntryId);

Review Comment:
   `tableConfig -> tableConfig.getTableName()`



-- 
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-unsubscribe@pinot.apache.org

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