[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10536: Retention manager hooks

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #10536:
URL: https://github.com/apache/pinot/pull/10536#discussion_r1161906009


##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -74,12 +75,17 @@ public class SegmentDeletionManager {
     RETENTION_DATE_FORMAT.setTimeZone(TimeZone.getTimeZone("UTC"));
   }
 
+  public interface SegmentDeletionHook {

Review Comment:
   Suggest calling it `SegmentDeletionListener` and rename the method to `onSegmentDeletion()`



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -166,6 +177,14 @@ protected synchronized void deleteSegmentFromPropertyStoreAndLocal(String tableN
         propStorePathList.add(segmentPropertyStorePath);
       }
 
+      for (SegmentDeletionHook segmentDeletionHook : _segmentDeletionHooks) {
+        try {
+          segmentDeletionHook.process(tableName, segmentsToDelete);
+        } catch (Exception e) {
+          LOGGER.error("Failed to process segment deletion hook: {}", segmentDeletionHook, e);

Review Comment:
   Update the message accordingly if we decide to change the method name



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -74,12 +75,17 @@ public class SegmentDeletionManager {
     RETENTION_DATE_FORMAT.setTimeZone(TimeZone.getTimeZone("UTC"));
   }
 
+  public interface SegmentDeletionHook {
+    void process(String tableName, List<String> segmentsToDelete);
+  }
+
   private final ScheduledExecutorService _executorService;
   private final String _dataDir;
   private final String _helixClusterName;
   private final HelixAdmin _helixAdmin;
   private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
   private final long _defaultDeletedSegmentsRetentionMs;
+  private final ConcurrentLinkedQueue<SegmentDeletionHook> _segmentDeletionHooks;

Review Comment:
   We probably don't need a concurrent queue since the register should happen before the start (we might want to introduce `start()` to start the background task)



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