jtao15 commented on a change in pull request #7368:
URL: https://github.com/apache/pinot/pull/7368#discussion_r696993017
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -110,6 +111,7 @@ public String getTaskType() {
public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
String taskType = MergeRollupTask.TASK_TYPE;
List<PinotTaskConfig> pinotTaskConfigs = new ArrayList<>();
+ Map<String, Integer> mergeRollupTaskDelayInNumBuckets = new HashMap<>();
Review comment:
We don't have to create a map here? We can compute all the metrics from
the cached `mergeRollupTaskMetadata` when we try to write the metadata to
zookeeper?
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -328,6 +334,14 @@ public String getTaskType() {
// Write updated watermark map to zookeeper
try {
_clusterInfoAccessor.setMergeRollupTaskMetadata(mergeRollupTaskMetadata,
expectedVersion);
+ for (Map.Entry<String, Integer> entry :
mergeRollupTaskDelayInNumBuckets.entrySet()) {
Review comment:
This will work without creating the map
`mergeRollupTaskDelayInNumBuckets`?
```
for (Map.Entry<String, Long> entry :
mergeRollupTaskMetadata.getWatermarkMap()) {
String metricKey = offlineTableName + "." + entry.getKey();
int delayInNumTimeBuckets =
getMergeRollupTaskDelayInNumTimeBuckets(entry.getValue(), bufferMs, bucketMs));
// Update gauge value that indicates the delay in terms of the number of
time buckets.
// e.g. mergeRollupTaskDelayInNumBuckets.myTable_OFFLINE.daily
ControllerMetrics controllerMetrics =
_clusterInfoAccessor.getControllerMetrics();
controllerMetrics.setValueOfTableGauge(metricKey,
ControllerGauge.MERGE_ROLLUP_TASK_DELAY_IN_NUM_BUCKETS,
entry.getValue());
}
```
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -277,6 +279,10 @@ public String getTaskType() {
}
Long prevWatermarkMs =
mergeRollupTaskMetadata.getWatermarkMap().put(mergeLevel, windowStartMs);
+ if (prevWatermarkMs != null) {
Review comment:
+1
--
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
