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

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

mcvsubbu commented on code in PR #10536:
URL: https://github.com/apache/pinot/pull/10536#discussion_r1163278667


##########
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 SegmentDeletionListener {

Review Comment:
   This should be in `spi`



##########
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 (SegmentDeletionListener segmentDeletionListener : _segmentDeletionListeners) {
+        try {
+          segmentDeletionListener.onSegmentDeletion(tableName, segmentsToDelete);

Review Comment:
   Does it make sense to change it to a Collection in the interface?
   
   Also, it may be useful to have other data along with segment name (start/end time, or creation time, etc.). So, does it make sense to make this a list of descriptors? Or, a list of segment metadata?



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -88,6 +94,7 @@ public SegmentDeletionManager(String dataDir, HelixAdmin helixAdmin, String heli
     _helixClusterName = helixClusterName;
     _propertyStore = propertyStore;
     _defaultDeletedSegmentsRetentionMs = TimeUnit.DAYS.toMillis(deletedSegmentsRetentionInDays);
+    _segmentDeletionListeners = new ConcurrentLinkedQueue<>();

Review Comment:
   I have the same comment as Jackie. I am not sure why there is a concurrent queue here



##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -99,6 +106,10 @@ public Thread newThread(Runnable runnable) {
     });
   }
 
+  public void registerSegmentDeletionListener(SegmentDeletionListener segmentDeletionListener) {

Review Comment:
   Should be during start up time



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