Jackie-Jiang commented on code in PR #10815:
URL: https://github.com/apache/pinot/pull/10815#discussion_r1323752263
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -215,9 +215,15 @@ public static class ControllerPeriodicTasksConf {
"controller.realtime.segment.deepStoreUploadRetryEnabled";
public static final String DEEP_STORE_RETRY_UPLOAD_TIMEOUT_MS =
"controller.realtime.segment.deepStoreUploadRetry.timeoutMs";
+ public static final String ENABLE_TMP_SEGMENT_ASYNC_DELETION =
+ "controller.realtime.split.commit.segment.tmp.cleanup.async.enable";
+ // temporary segments within expiration won't be deleted so that ongoing
split commit won't be impacted.
+ public static final String SPLIT_COMMIT_TMP_SEGMENT_LIFETIME_SECONDS =
+ "controller.realtime.split.commit.segment.tmp.lifetime.seconds";
Review Comment:
Same here
```suggestion
public static final String TMP_SEGMENT_RETENTION_IN_SECONDS =
"controller.realtime.segment.tmpFileRetentionInSeconds";
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -117,8 +117,18 @@ protected void processTable(String tableNameWithType,
Context context) {
private void runSegmentLevelValidation(TableConfig tableConfig,
PartitionLevelStreamConfig streamConfig) {
String realtimeTableName = tableConfig.getTableName();
+
List<SegmentZKMetadata> segmentsZKMetadata =
_pinotHelixResourceManager.getSegmentsZKMetadata(realtimeTableName);
+ // Delete tmp segments
+ try {
+ long numDeleteTmpSegments =
_llcRealtimeSegmentManager.deleteTmpSegments(realtimeTableName,
segmentsZKMetadata);
+ LOGGER.info("Delete {} tmp segments for table {}", numDeleteTmpSegments,
realtimeTableName);
+ _validationMetrics.updateTmpSegmentCountGauge(realtimeTableName,
numDeleteTmpSegments);
+ } catch (Exception e) {
+ LOGGER.error(String.format("Fail to delete tmp segments for table %s",
realtimeTableName), e);
Review Comment:
```suggestion
LOGGER.error("Failed to delete tmp segments for table: {}",
realtimeTableName, e);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -215,9 +215,15 @@ public static class ControllerPeriodicTasksConf {
"controller.realtime.segment.deepStoreUploadRetryEnabled";
public static final String DEEP_STORE_RETRY_UPLOAD_TIMEOUT_MS =
"controller.realtime.segment.deepStoreUploadRetry.timeoutMs";
+ public static final String ENABLE_TMP_SEGMENT_ASYNC_DELETION =
+ "controller.realtime.split.commit.segment.tmp.cleanup.async.enable";
Review Comment:
The tmp file is not generated only for split commit case. Suggest renaming
the keep to match other configs
```suggestion
"controller.realtime.segment.tmpFileAsyncDeletionEnabled";
```
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ValidationMetrics.java:
##########
@@ -239,6 +239,14 @@ public void cleanupSegmentCountGauge(final String
resource) {
removeGauge(resource, ValidationMetricName.SEGMENT_COUNT);
}
+ public void updateTmpSegmentCountGauge(final String resource, final long
tmpSegmentCount) {
Review Comment:
Why do we define it as a gauge? Should it be a meter instead?
Let's match the name of the method and the metric
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -117,8 +117,18 @@ protected void processTable(String tableNameWithType,
Context context) {
private void runSegmentLevelValidation(TableConfig tableConfig,
PartitionLevelStreamConfig streamConfig) {
String realtimeTableName = tableConfig.getTableName();
+
List<SegmentZKMetadata> segmentsZKMetadata =
_pinotHelixResourceManager.getSegmentsZKMetadata(realtimeTableName);
+ // Delete tmp segments
+ try {
+ long numDeleteTmpSegments =
_llcRealtimeSegmentManager.deleteTmpSegments(realtimeTableName,
segmentsZKMetadata);
+ LOGGER.info("Delete {} tmp segments for table {}", numDeleteTmpSegments,
realtimeTableName);
Review Comment:
(nit)
```suggestion
LOGGER.info("Deleted {} tmp segments for table: {}",
numDeleteTmpSegments, realtimeTableName);
```
--
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
