[GitHub] [pinot] snleee commented on a change in pull request #7617: Use maxEndTimeMs for merge/roll-up delay metrics.

[GitBox <gi...@apache.org>]

snleee commented on a change in pull request #7617:
URL: https://github.com/apache/pinot/pull/7617#discussion_r734900769



##########
File path: pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -110,21 +110,18 @@
   // number to be 7 and merge task is configured with "bucketTimePeriod = 1d", this means that we have 7 days of
   // delay. When operating merge/roll-up task in production, we should set the alert on this metrics to find out the
   // delay. Setting the alert on 7 time buckets of delay would be a good starting point.
-  //
-  // NOTE: Based on the current scheduler logic, we are bumping up the watermark with some delay. (the current round
-  // will bump up the watermark for the window that got processed from the previous round). Due to this, we will
-  // correctly report the delay with one edge case. When we processed all available time windows, the watermark
-  // will not get bumped up until we schedule some task for the table. Due to this, we will always see the delay >= 1.
   private static final String MERGE_ROLLUP_TASK_DELAY_IN_NUM_BUCKETS = "mergeRollupTaskDelayInNumBuckets";
 
   // tableNameWithType -> mergeLevel -> watermarkMs
   private Map<String, Map<String, Long>> _mergeRollupWatermarks;
+  private Map<String, Long> _tableMaxEndTimeMs;

Review comment:
       can you add the comment on what's the key and the value?

##########
File path: pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -555,12 +557,13 @@ private long getMergeRollupTaskDelayInNumTimeBuckets(long watermarkMs, long buff
    *
    * @param tableNameWithType table name with type
    * @param mergeLevel merge level
+   * @param lowerMergeLevel lower merge level

Review comment:
       Please also update the doc for `maxEndTimeMs`

##########
File path: pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -254,7 +251,9 @@ public String getTaskType() {
         long bucketEndMs = bucketStartMs + bucketMs;
         // Create delay metrics even if there's no task scheduled, this helps the case that the controller is restarted
         // but the metrics are not available until the controller schedules a valid task
-        createOrUpdateDelayMetrics(offlineTableName, mergeLevel, watermarkMs, bufferMs, bucketMs);
+        long maxEndTimeMs = preSelectedSegments.get(preSelectedSegments.size() - 1).getEndTimeMs();

Review comment:
       I think that the last entry may not contain the maximum end timestamp given that we sort based on `start, end`.  For instance, think of 2 timestamp pairs: (1, 5), (2, 4). The last element can have a smaller `end` value.
   
   We probably need to loop through the segment to compute the max end time.




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