Copilot commented on code in PR #16368:
URL: https://github.com/apache/pinot/pull/16368#discussion_r2220371925
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskGenerator.java:
##########
@@ -141,6 +146,13 @@ public List<PinotTaskConfig>
generateTasks(List<TableConfig> tableConfigs) {
pinotTaskConfigs.add(new PinotTaskConfig(taskType, configs));
tableNumTasks++;
}
+ if (!segmentsForDeletion.isEmpty()) {
+
_clusterInfoAccessor.getPinotHelixResourceManager().deleteSegments(tableName,
segmentsForDeletion,
+ "0d");
Review Comment:
The hardcoded retention value "0d" should be extracted as a constant or made
configurable. Magic strings reduce maintainability and make the code harder to
understand.
```suggestion
RETENTION_PERIOD_FOR_DELETION);
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/PurgeMinionClusterIntegrationTest.java:
##########
@@ -151,6 +152,9 @@ private void setRecordPurger() {
PURGE_OLD_SEGMENTS_WITH_NEW_INDICES_TABLE);
if (tableNames.contains(rawTableName)) {
return row -> row.getValue("ArrTime").equals(1);
+ } else if (PURGE_ALL_RECORDS_TABLE.equals(rawTableName)) {
+ // Purge ALL records to test segment deletion
Review Comment:
[nitpick] The lambda expression 'row -> true' could benefit from a more
descriptive comment explaining that this purges all records for the
PURGE_ALL_RECORDS_TABLE test case.
```suggestion
// Purge ALL records for the PURGE_ALL_RECORDS_TABLE test case.
// This is done to simulate a scenario where all records are removed,
// allowing us to test the behavior of segment deletion when no
records remain.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
