sajjad-moradi commented on code in PR #12710:
URL: https://github.com/apache/pinot/pull/12710#discussion_r1539860670
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java:
##########
@@ -68,4 +71,10 @@ protected long getSegmentCrc(String tableNameWithType,
String segmentName) {
*/
return segmentZKMetadata == null ? -1 : segmentZKMetadata.getCrc();
}
+
+ public static void addTaskMeterMetrics(MinionMeter meter, long unitCount,
String tableName, String taskType) {
+ MinionMetrics.get().addMeteredGlobalValue(meter, unitCount);
+ MinionMetrics.get().addMeteredTableValue(tableName, meter, unitCount);
+ MinionMetrics.get().addMeteredTableValue(tableName, taskType, meter,
unitCount);
Review Comment:
`MinionMetrics.get()` -> `_minionMetrics` ?
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskExecutor.java:
##########
@@ -77,6 +79,11 @@ protected SegmentConversionResult convert(PinotTaskConfig
pinotTaskConfig, File
purgedSegmentFile = indexDir;
}
+ BaseTaskExecutor.addTaskMeterMetrics(MinionMeter.RECORDS_PER_SEGMENT,
segmentPurger.getTotalRecordsProcessed(),
+ tableNameWithType, taskType);
Review Comment:
We can add this line to `BaseSingleSegmentConversionExecutor`, so that
there's no need to repeat in each subclass (here and the upsert one). Also if
we do this, there's no need to add `getTotalRecordsProcessed` method to
segmentPurger.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java:
##########
@@ -31,7 +31,13 @@ public enum MinionMeter implements AbstractMetrics.Meter {
NUMBER_TASKS_FAILED("tasks", false),
NUMBER_TASKS_FATAL_FAILED("tasks", false),
SEGMENT_UPLOAD_FAIL_COUNT("segments", false),
- SEGMENT_DOWNLOAD_FAIL_COUNT("segments", false);
+ SEGMENT_DOWNLOAD_FAIL_COUNT("segments", false),
+ SEGMENTS_DOWNLOADED("segments", false),
+ SEGMENTS_UPLOADED("segments", false),
+ SEGMENT_SIZE_DOWNLOADED("bytes", false),
+ SEGMENT_SIZE_UPLOADED("bytes", false),
+ RECORDS_PER_SEGMENT("rows", false),
Review Comment:
+1
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -123,6 +120,11 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
LOGGER.warn("Failed to delete tarred input segment: {}",
tarredSegmentFile.getAbsolutePath());
}
+ // Segment download metrics
+ long downloadSegmentSize = FileUtils.sizeOfDirectory(indexDir);
+ addTaskMeterMetrics(MinionMeter.SEGMENT_SIZE_DOWNLOADED,
downloadSegmentSize, tableNameWithType, taskType);
+ addTaskMeterMetrics(MinionMeter.SEGMENTS_DOWNLOADED, 1L,
tableNameWithType, taskType);
Review Comment:
This is repeated in both single and multiple executors. If someone later
adds a new metric to one of these classes, they need to remember to add it to
the other as well. We can refactor this part to a function in BaseTaskExecutor,
and just call it here.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -135,6 +137,12 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
if (convertedSegmentDir == null) {
return segmentConversionResult;
}
+
+ // Segment upload metrics
+ long uploadSegmentSize = FileUtils.sizeOfDirectory(workingDir);
+ addTaskMeterMetrics(MinionMeter.SEGMENT_SIZE_UPLOADED,
uploadSegmentSize, tableNameWithType, taskType);
+ addTaskMeterMetrics(MinionMeter.SEGMENTS_UPLOADED, 1L,
tableNameWithType, taskType);
Review Comment:
Same for this one.
--
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]
