[GitHub] [pinot] walterddr commented on a change in pull request #8078: Segment retention for table

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

walterddr commented on a change in pull request #8078:
URL: https://github.com/apache/pinot/pull/8078#discussion_r798796933



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -703,7 +703,9 @@ public synchronized PinotResourceManagerResponse deleteSegments(String tableName
       Preconditions.checkArgument(TableNameBuilder.isTableResource(tableNameWithType),
           "Table name: %s is not a valid table name with type suffix", tableNameWithType);
       HelixHelper.removeSegmentsFromIdealState(_helixZkManager, tableNameWithType, segmentNames);
-      _segmentDeletionManager.deleteSegments(tableNameWithType, segmentNames);
+      TableConfig tableConfig = ZKMetadataProvider.getTableConfig(_propertyStore, tableNameWithType);
+      long retentionMs = _segmentDeletionManager.getRetentionFromTableConfig(tableConfig);
+      _segmentDeletionManager.deleteSegments(tableNameWithType, segmentNames, retentionMs);

Review comment:
       IMO passing in tableConfig will be more confusing because, it is possible to use a retentionMs that is not coming from tableConfig; and also there's no point passing in table name separately either. 
   
   Let's not restrict the API to only work on tableConfig. segmentDeletionManager only needs a zkpath and deep store path to work IMO. 

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
##########
@@ -29,6 +29,8 @@
 public class SegmentsValidationAndRetentionConfig extends BaseJsonConfig {
   private String _retentionTimeUnit;
   private String _retentionTimeValue;
+  private String _deletedSegmentRetentionTimeUnit;

Review comment:
       would like to keep this consistent with the `retentionTime` setting for now, wdyt




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