fix: Limit number of unused segment rows scanned - #19690
Conversation
cryptoe
left a comment
cryptoe
left a comment
There was a problem hiding this comment.
Changes look good to me.
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Main code LGTM, I had some comments about the comments and naming.
| + " GROUP BY %2$send%2$s, start" | ||
| + " %3$s", | ||
| dbTables.getSegmentsTable(), connector.getQuoteString(), connector.limitClause(limit) | ||
| // Text blocks are not supported in the current checkstyle version |
There was a problem hiding this comment.
I believe this comment is inaccurate. We use text blocks elsewhere. Maybe there's some other reason that checkstyle flags this block. The suppression is for Regexp so it could be one of our custom rules.
There was a problem hiding this comment.
Yes, it is one of the custom rules. I will try to update it in a later PR.
Thanks for calling this out, I will fix the comment.
| * list contains the earliest or latest intervals present in the datasource. | ||
| * Gets unused segment intervals for the specified datasource. | ||
| * <p> | ||
| * Note: This method does NOT guarantee that: |
There was a problem hiding this comment.
I see why we need to weaken the guarantees. However, I think it still does guarantee that if there are unused segments, then we will return at least one interval. This is a useful guarantee that should be mentioned.
| * | ||
| * @return List of unused segment intervals containing upto {@code limit} interval entries. | ||
| */ | ||
| public List<Interval> retrieveUnusedSegmentIntervals(String dataSource, int limit) |
There was a problem hiding this comment.
Consider renaming it to retrieveSomeUnusedSegmentIntervals, just to emphasize that not all of them will necessarily be returned.
|
Thanks for the reviews, @cryptoe , @gianm , @capistrant ! |
4 participants
Related to #19685
Description
In large clusters with a large number of unused segments for a single datasource, the
UnusedSegmentsKillermay error out while fetching the list of unused segment intervals.Fix
UnusedSegmentsKillerto 1MThis PR has: