Jackie-Jiang commented on a change in pull request #8176:
URL: https://github.com/apache/pinot/pull/8176#discussion_r809455402
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -53,22 +58,34 @@
private static final Logger LOGGER =
LoggerFactory.getLogger(SegmentDeletionManager.class);
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;
+
+ // Retention date format will be written as suffix to deleted segments under
`Deleted_Segments` folder. for example:
+ //
`Deleted_Segments/myTable/myTable_mySegment_0__RETENTION_UNTIL__20220202_120000`
to indicate that this segment
Review comment:
(minor) Update the comment
```suggestion
//
`Deleted_Segments/myTable/myTable_mySegment_0__RETENTION_UNTIL__202202021200`
to indicate that this segment
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -84,22 +101,29 @@ public void stop() {
_executorService.shutdownNow();
}
- public void deleteSegments(final String tableName, final Collection<String>
segmentIds) {
- deleteSegmentsWithDelay(tableName, segmentIds,
DEFAULT_DELETION_DELAY_SECONDS);
+ public void deleteSegments(String tableName, Collection<String> segmentIds) {
+ deleteSegments(tableName, segmentIds, null);
}
- protected void deleteSegmentsWithDelay(final String tableName, final
Collection<String> segmentIds,
- final long deletionDelaySeconds) {
+ public void deleteSegments(String tableName, Collection<String> segmentIds,
+ TableConfig tableConfig) {
Review comment:
```suggestion
@Nullable TableConfig tableConfig) {
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -160,18 +184,29 @@ protected synchronized void
deleteSegmentFromPropertyStoreAndLocal(String tableN
if (!segmentsToRetryLater.isEmpty()) {
long effectiveDeletionDelay = Math.min(deletionDelay * 2,
MAX_DELETION_DELAY_SECONDS);
LOGGER.info("Postponing deletion of {} segments from table {}",
segmentsToRetryLater.size(), tableName);
- deleteSegmentsWithDelay(tableName, segmentsToRetryLater,
effectiveDeletionDelay);
+ deleteSegmentsWithDelay(tableName, segmentsToRetryLater,
deletedSegmentsRetentionMs, effectiveDeletionDelay);
return;
}
}
public void removeSegmentsFromStore(String tableNameWithType, List<String>
segments) {
+ removeSegmentsFromStore(tableNameWithType, segments,
_defaultDeletedSegmentsRetentionMs, true);
+ }
+
+ public void removeSegmentsFromStore(String tableNameWithType, List<String>
segments,
+ long deletedSegmentsRetentionMs) {
+ removeSegmentsFromStore(tableNameWithType, segments,
deletedSegmentsRetentionMs, false);
+ }
+
+ public void removeSegmentsFromStore(String tableNameWithType, List<String>
segments,
+ long deletedSegmentsRetentionMs, boolean usedDefaultClusterRetention) {
Review comment:
We don't need `usedDefaultClusterRetention` if we change
`deletedSegmentsRetentionMs` to `@Nullable Long`
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java
##########
@@ -250,18 +274,83 @@ public void testRemoveDeletedSegments()
// Check that dummy directories and files are successfully created.
Assert.assertEquals(dummyDir1.list().length, 3);
Assert.assertEquals(dummyDir2.list().length, 3);
+ Assert.assertEquals(dummyDir3.list().length, 3);
- // Try to remove files with the retention of 3 days.
- deletionManager.removeAgedDeletedSegments(3);
- Assert.assertEquals(dummyDir1.list().length, 3);
- Assert.assertEquals(dummyDir2.list().length, 1);
+ // Try to remove files with the retention of 1 days.
+ deletionManager.removeAgedDeletedSegments();
- // Try to further remove files with the retention of 1 days.
- deletionManager.removeAgedDeletedSegments(1);
+ // Check that only 1 day retention file is remaining
Assert.assertEquals(dummyDir1.list().length, 1);
// Check that empty directory has successfully been removed.
Assert.assertEquals(dummyDir2.exists(), false);
+
+ // Check that deleted file without retention suffix is honoring
cluster-wide retention period of 7 days.
+ Assert.assertEquals(dummyDir3.list().length, 1);
+ }
+
+ @Test
+ public void testSegmentDeletionLogic()
+ throws Exception {
+ Map<String, Object> properties = new HashMap<>();
+
properties.put(CommonConstants.Controller.PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY
+ ".class",
+ LocalPinotFS.class.getName());
+ PinotFSFactory.init(new PinotConfiguration(properties));
+
+ HelixAdmin helixAdmin = makeHelixAdmin();
+ ZkHelixPropertyStore<ZNRecord> propertyStore = makePropertyStore();
+ File tempDir = Files.createTempDir();
+ tempDir.deleteOnExit();
+ SegmentDeletionManager deletionManager = new SegmentDeletionManager(
+ tempDir.getAbsolutePath(), helixAdmin, CLUSTER_NAME, propertyStore, 7);
+
+ // create table segment files.
+ Set<String> segments = new HashSet<>(segmentsThatShouldBeDeleted());
+ createTableAndSegmentFiles(tempDir, segmentsThatShouldBeDeleted());
+ File tableDir = new File(tempDir.getAbsolutePath() + File.separator +
TABLE_NAME);
+ File deletedTableDir = new File(tempDir.getAbsolutePath() + File.separator
+ "Deleted_Segments"
+ + File.separator + TABLE_NAME);
+
+ // delete the segments instantly.
+ SegmentsValidationAndRetentionConfig mockValidationConfig =
mock(SegmentsValidationAndRetentionConfig.class);
+
when(mockValidationConfig.getDeletedSegmentsRetentionPeriod()).thenReturn("0d");
+ TableConfig mockTableConfig = mock(TableConfig.class);
+
when(mockTableConfig.getValidationConfig()).thenReturn(mockValidationConfig);
+ deletionManager.deleteSegments(TABLE_NAME, segments, mockTableConfig);
+
+ // Sleep 3 second to ensure the async delete actually kicked in.
+ Thread.sleep(3000L);
Review comment:
This can potentially be flaky. Suggest using
`TestUtils.waitForCondition()` with a longer timeout
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
##########
@@ -84,22 +101,29 @@ public void stop() {
_executorService.shutdownNow();
}
- public void deleteSegments(final String tableName, final Collection<String>
segmentIds) {
- deleteSegmentsWithDelay(tableName, segmentIds,
DEFAULT_DELETION_DELAY_SECONDS);
+ public void deleteSegments(String tableName, Collection<String> segmentIds) {
+ deleteSegments(tableName, segmentIds, null);
}
- protected void deleteSegmentsWithDelay(final String tableName, final
Collection<String> segmentIds,
- final long deletionDelaySeconds) {
+ public void deleteSegments(String tableName, Collection<String> segmentIds,
+ TableConfig tableConfig) {
+ long deletedSegmentsRetentionMs =
getRetentionMsFromTableConfig(tableConfig);
+ deleteSegmentsWithDelay(tableName, segmentIds, deletedSegmentsRetentionMs,
DEFAULT_DELETION_DELAY_SECONDS);
Review comment:
This won't give expected behavior if table config doesn't override the
retention because it will set `usedDefaultClusterRetention` to `false` in
`removeSegmentFromStore()`. One way to fix it is to return `Long` in
`getRetentionMsFromTableConfig()` (you can make it static), and if there is no
override, return `null`. In all other method, make `deletedSegmentsRetentionMs`
`Long`, and use default retention if it is `null`
--
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
