[GitHub] [pinot] jtao15 commented on a diff in pull request #10754: Keep last completed segments for retention

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

jtao15 commented on code in PR #10754:
URL: https://github.com/apache/pinot/pull/10754#discussion_r1196876283


##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java:
##########
@@ -233,6 +233,39 @@ public void testRealtimeLLCCleanup()
     verify(pinotHelixResourceManager, times(1)).deleteSegments(anyString(), anyList());
   }
 
+  // This test makes sure that we do not clean up last llc completed segments
+  @Test
+  public void testRealtimeLastLLCCleanup()
+      throws Exception {
+    final long now = System.currentTimeMillis();
+    final int replicaCount = 1;
+
+    TableConfig tableConfig = createRealtimeTableConfig1(replicaCount);
+    List<String> removedSegments = new ArrayList<>();
+    LeadControllerManager leadControllerManager = mock(LeadControllerManager.class);
+    when(leadControllerManager.isLeaderForTable(anyString())).thenReturn(true);
+    PinotHelixResourceManager pinotHelixResourceManager =
+        setupSegmentMetadataForPausedTable(tableConfig, now, removedSegments);
+    setupPinotHelixResourceManager(tableConfig, removedSegments, pinotHelixResourceManager, leadControllerManager);
+
+    ControllerConf conf = new ControllerConf();
+    ControllerMetrics controllerMetrics = new ControllerMetrics(PinotMetricUtils.getPinotMetricsRegistry());
+    conf.setRetentionControllerFrequencyInSeconds(0);
+    conf.setDeletedSegmentsRetentionInDays(0);
+    RetentionManager retentionManager =
+        new RetentionManager(pinotHelixResourceManager, leadControllerManager, conf, controllerMetrics);
+    retentionManager.start();
+    retentionManager.run();
+
+    SegmentDeletionManager deletionManager = pinotHelixResourceManager.getSegmentDeletionManager();
+
+    // Verify that the removeAgedDeletedSegments() method in deletion manager is actually called.
+    verify(deletionManager, times(1)).removeAgedDeletedSegments(leadControllerManager);
+
+    // Verify that the deleteSegments method is actually called.
+    verify(pinotHelixResourceManager, times(1)).deleteSegments(anyString(), anyList());

Review Comment:
   The deleted segments are checked in the setupPinotHelixResourceManager() as following, so I think this is not needed.
   ```
   // If and when PinotHelixResourceManager.deleteSegments() is invoked, make sure that the segments deleted
       // are exactly the same as the ones we expect to be deleted.
       doAnswer(new Answer() {
         @Override
         public Object answer(InvocationOnMock invocationOnMock)
             throws Throwable {
           Object[] args = invocationOnMock.getArguments();
           String tableNameArg = (String) args[0];
           Assert.assertEquals(tableNameArg, tableNameWithType);
           List<String> segmentListArg = (List<String>) args[1];
           Assert.assertEquals(segmentListArg.size(), removedSegments.size());
           for (String segmentName : removedSegments) {
             Assert.assertTrue(segmentListArg.contains(segmentName));
           }
           return null;
         }
       }).when(resourceManager).deleteSegments(anyString(), ArgumentMatchers.anyList());
   ```



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