-
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
Use task actions to fetch used segments in MSQ #15284
Use task actions to fetch used segments in MSQ #15284
Conversation
@AmatyaAvadhanula , are more changes pending in this PR? |
@kfaraz, yes, there are several failing tests which need to be fixed. |
// If the task is operating with a REPLACE lock, | ||
// any segment created after the lock was acquired for its interval will not be considered. | ||
final Collection<DataSegment> publishedUsedSegments = new HashSet<>(); | ||
for (Interval interval : intervals) { |
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.
Looking at RetrieveSegmentsToReplaceAction
and the method IndexerMetadataStorageConnector.retrieveUsedSegmentsAndCreatedDates
, I suppose we could just pass a list of intervals to the action rather than firing a separate action for each interval.
...-query/src/test/java/org/apache/druid/msq/sql/resources/SqlMSQStatementResourcePostTest.java
Outdated
Show resolved
Hide resolved
This reverts commit 95ab649
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.
Minor feedback which can be addressed in a later PR, otherwise LGTM.
@@ -122,6 +128,14 @@ public <RetType> RetType submit(TaskAction<RetType> taskAction) | |||
.build() | |||
).collect(Collectors.toSet()); | |||
} | |||
} else if (taskAction instanceof RetrieveSegmentsToReplaceAction) { | |||
String dataSource = ((RetrieveSegmentsToReplaceAction) taskAction).getDataSource(); | |||
return (RetType) injector.getInstance(SpecificSegmentsQuerySegmentWalker.class) |
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.
Could you please add a comment here explaining why we need to use the QuerySegmentWalker
here instead of how we handle RetrieveUsedSegmentsAction
here or how we would respond to the coordinatorClient().fetchUsedSegments()
in MSQ tests?
* Use task actions to fetch used segments in MSQ * Fix tests * Fixing tests. * Revert "Fix tests" This reverts commit 95ab649 * Removing conditional check in tests. * Pulling in latest changes. --------- Co-authored-by: cryptoe <[email protected]>
* Use task actions to fetch used segments in MSQ * Fix tests * Fixing tests. * Revert "Fix tests" This reverts commit 95ab649 * Removing conditional check in tests. * Pulling in latest changes. --------- Co-authored-by: cryptoe <[email protected]>
#15220 Introduced a new
RetrieveSegmentsToReplace
action to be used in the DruidInputSource.This PR lets the MSQ Controller use this action in ControllerImpl#makeDataSegmentTimelineView, to allow a consistent set of segments to be returned, especially with a REPLACE lock.