Jackie-Jiang commented on a change in pull request #8176:
URL: https://github.com/apache/pinot/pull/8176#discussion_r803959887
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
+ String[] split = targetFile.split(RETENTION_PERIOD_SEPARATOR);
+ if (split.length > 1) {
+ try {
+ return Long.parseLong(split[split.length - 1]);
+ } catch (Exception e) {
+ LOGGER.warn(String.format("No retention suffix found for file %s,
using default %d", targetFile,
+ _defaultDeletedSegmentsRetentionMs));
+ }
+ }
+ return _defaultDeletedSegmentsRetentionMs;
Review comment:
Please add some comments here explaining this is for
backward-compatibility
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
+ String[] split = targetFile.split(RETENTION_PERIOD_SEPARATOR);
+ if (split.length > 1) {
+ try {
+ return Long.parseLong(split[split.length - 1]);
+ } catch (Exception e) {
+ LOGGER.warn(String.format("No retention suffix found for file %s,
using default %d", targetFile,
+ _defaultDeletedSegmentsRetentionMs));
+ }
+ }
+ return _defaultDeletedSegmentsRetentionMs;
+ }
+
+ public long getRetentionMsFromTableConfig(TableConfig tableConfig) {
+ long retentionMs = _defaultDeletedSegmentsRetentionMs;
+ if (tableConfig != null && tableConfig.getValidationConfig() != null) {
Review comment:
(minor) This 2 checks are redundant. Both of them can never be `null`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
+ String[] split = targetFile.split(RETENTION_PERIOD_SEPARATOR);
+ if (split.length > 1) {
+ try {
+ return Long.parseLong(split[split.length - 1]);
+ } catch (Exception e) {
+ LOGGER.warn(String.format("No retention suffix found for file %s,
using default %d", targetFile,
+ _defaultDeletedSegmentsRetentionMs));
+ }
+ }
+ return _defaultDeletedSegmentsRetentionMs;
+ }
+
+ public long getRetentionMsFromTableConfig(TableConfig tableConfig) {
+ long retentionMs = _defaultDeletedSegmentsRetentionMs;
+ if (tableConfig != null && tableConfig.getValidationConfig() != null) {
+ SegmentsValidationAndRetentionConfig validationConfig =
tableConfig.getValidationConfig();
+ if
(!StringUtils.isEmpty(validationConfig.getDeletedSegmentRetentionPeriod())) {
+ try {
+ retentionMs =
TimeUtils.convertPeriodToMillis(validationConfig.getDeletedSegmentRetentionPeriod());
+ } catch (Exception e) {
+ LOGGER.warn(String.format("Unable to parse deleted segment retention
config for table %s, using to default "
+ + "retention value %dms", tableConfig.getTableName(),
_defaultDeletedSegmentsRetentionMs), e);
Review comment:
```suggestion
LOGGER.warn("Unable to parse deleted segment retention config for
table: {}, using default: {}", tableConfig.getTableName(),
_defaultDeletedSegmentsRetentionMs, e);
```
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java
##########
@@ -67,6 +68,7 @@
private boolean _allowNullTimeValue;
private String _retentionTimeUnit;
private String _retentionTimeValue;
+ private String _deletedSegmentRetentionPeriod =
DEFAULT_SEGMENT_DELETION_RETENTION_PERIOD;
Review comment:
```suggestion
private String _deletedSegmentsRetentionPeriod =
DEFAULT_DELETED_SEGMENTS_RETENTION_PERIOD;
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -54,21 +58,22 @@
private static final long MAX_DELETION_DELAY_SECONDS = 300L; // Maximum of
5 minutes back-off to retry the deletion
private static final long DEFAULT_DELETION_DELAY_SECONDS = 2L;
private static final String DELETED_SEGMENTS = "Deleted_Segments";
+ private static final String RETENTION_PERIOD_SEPARATOR = "__RETENTION_MS__";
Review comment:
The suffix is ms, not period
```suggestion
private static final String RETENTION_MS_SEPARATOR = "__RETENTION_MS__";
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
+ String[] split = targetFile.split(RETENTION_PERIOD_SEPARATOR);
+ if (split.length > 1) {
+ try {
+ return Long.parseLong(split[split.length - 1]);
+ } catch (Exception e) {
+ LOGGER.warn(String.format("No retention suffix found for file %s,
using default %d", targetFile,
+ _defaultDeletedSegmentsRetentionMs));
Review comment:
```suggestion
LOGGER.warn("No retention suffix found for file: {}, using default:
{}", targetFile, _defaultDeletedSegmentsRetentionMs);
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
Review comment:
```suggestion
private long getRetentionMsFromFile(String targetFile) {
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
+ return fileName + RETENTION_PERIOD_SEPARATOR + deletedSegmentsRetentionMs;
+ }
+
+ private long getRetentionPeriodFromFile(String targetFile) {
+ String[] split = targetFile.split(RETENTION_PERIOD_SEPARATOR);
Review comment:
Use `StringUtils.split()` to avoid regex matching
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java
##########
@@ -97,8 +93,8 @@ protected void processTable(String tableNameWithType) {
@Override
protected void postprocess() {
- LOGGER.info("Removing aged (more than {} days) deleted segments for all
tables", _deletedSegmentsRetentionInDays);
-
_pinotHelixResourceManager.getSegmentDeletionManager().removeAgedDeletedSegments(_deletedSegmentsRetentionInDays);
+ LOGGER.info("Clean up aged deleted segments for all tables");
Review comment:
(minor)
```suggestion
LOGGER.info("Removing aged deleted segments for all tables");
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -278,4 +294,37 @@ public void removeAgedDeletedSegments(int retentionInDays)
{
LOGGER.info("dataDir is not configured, won't delete any expired
segments from deleted directory.");
}
}
+
+ private String getRetentionSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
Review comment:
```suggestion
private String getDeletedSegmentFileName(String fileName, long
deletedSegmentsRetentionMs) {
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -254,7 +270,7 @@ public void removeAgedDeletedSegments(int retentionInDays) {
int numFilesDeleted = 0;
for (String targetFile : targetFiles) {
URI targetURI = URIUtils.getUri(targetFile);
- Date dateToDelete =
DateTime.now().minusDays(retentionInDays).toDate();
+ Date dateToDelete =
DateTime.now().minus(getRetentionPeriodFromFile(targetFile)).toDate();
Review comment:
Not introduced in this PR, but can be simplified:
```suggestion
long deletionTimeMs = System.currentTimeMillis() -
getRetentionMsFromFile(targetFile);
```
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java
##########
@@ -29,6 +29,7 @@
public class SegmentsValidationAndRetentionConfig extends BaseJsonConfig {
private String _retentionTimeUnit;
private String _retentionTimeValue;
+ private String _deletedSegmentRetentionPeriod;
Review comment:
Also change the getter and setter
```suggestion
private String _deletedSegmentsRetentionPeriod;
```
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java
##########
@@ -50,6 +50,7 @@
private static final String DEFAULT_SEGMENT_PUSH_TYPE = "APPEND";
private static final String REFRESH_SEGMENT_PUSH_TYPE = "REFRESH";
private static final String DEFAULT_SEGMENT_ASSIGNMENT_STRATEGY =
"BalanceNumSegmentAssignmentStrategy";
+ private static final String DEFAULT_SEGMENT_DELETION_RETENTION_PERIOD = "7d";
Review comment:
To be consistent
```suggestion
private static final String DEFAULT_DELETED_SEGMENTS_RETENTION_PERIOD =
"7d";
```
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
