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: [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]
