-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing failing compaction/parallel index jobs during upgrade due to new actions being available on the overlord. #15430
Fixing failing compaction/parallel index jobs during upgrade due to new actions being available on the overlord. #15430
Conversation
…ew actions not available on the overlord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for catching this, @cryptoe ! The changes look good to me.
@@ -67,16 +68,16 @@ public class RetrieveSegmentsToReplaceAction implements TaskAction<Collection<Da | |||
private final String dataSource; | |||
|
|||
@JsonIgnore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had missed this earlier when this class was created. Not sure why we need @JsonIgnore
here.
@@ -84,19 +84,19 @@ default Collection<DataSegment> retrieveUsedSegmentsForInterval( | |||
/** | |||
* | |||
* Retrieve all published segments which are marked as used and the created_date of these segments belonging to the | |||
* given data source and interval from the metadata store. | |||
* given data source and List<Interval> from the metadata store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This doesn't render correctly in javadoc. Use intervals
or List of intervals
instead of this.
// Do not need an interval condition if the interval is ETERNITY | ||
if (!Intervals.isEternity(interval)) { | ||
intervals.add(interval); | ||
boolean intervalsAreEternity = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
boolean intervalsAreEternity = false; | |
boolean hasEternityInterval = false; |
I just pushed up a patch with the review comments. cc @kfaraz |
The error message that comes without this patch is
|
…ew actions being available on the overlord. (apache#15430) * Fixing failing compaction/parallel index jobs during upgrade due to new actions not available on the overlord. * Fixing build * Removing extra space. * Fixing json getter. * Review comments. (cherry picked from commit a018819)
…ew actions being available on the overlord. (apache#15430) * Fixing failing compaction/parallel index jobs during upgrade due to new actions not available on the overlord. * Fixing build * Removing extra space. * Fixing json getter. * Review comments.
…ew actions being available on the overlord. (apache#15430) * Fixing failing compaction/parallel index jobs during upgrade due to new actions not available on the overlord. * Fixing build * Removing extra space. * Fixing json getter. * Review comments.
With #15039, a new action got introduced on the overlord
RetrieveSegmentsToReplaceAction
. This might not be available on the overlord during upgrade resulting in compaction tasks/reindex tasks failing until the overlord is upgraded.I fixed it by adding an undocumented runtime property
enableConcurrentAppendAndReplace
which can be set by cluster admins in case they want to try out concurrentAppendAndReplace.This PR also changes
RetrieveSegmentsToReplaceAction
to take alist<Intervals>
as a input since that is required for MSQ jobs as discussed here : #15284 (comment)