Skip to content

Use task actions to fetch used segments in MSQ#15284

Merged
cryptoe merged 9 commits into
apache:masterfrom
AmatyaAvadhanula:msq_retrieveSegmentsToReplace
Dec 1, 2023
Merged

Use task actions to fetch used segments in MSQ#15284
cryptoe merged 9 commits into
apache:masterfrom
AmatyaAvadhanula:msq_retrieveSegmentsToReplace

Conversation

@AmatyaAvadhanula

Copy link
Copy Markdown
Contributor

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

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@github-actions github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 31, 2023
@kfaraz

kfaraz commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

@AmatyaAvadhanula , are more changes pending in this PR?

@AmatyaAvadhanula

Copy link
Copy Markdown
Contributor Author

@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) {

Copy link
Copy Markdown
Contributor

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.

@cryptoe
cryptoe marked this pull request as ready for review November 21, 2023 15:25

@kfaraz kfaraz left a comment

Copy link
Copy Markdown
Contributor

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.

}
} else if (taskAction instanceof RetrieveSegmentsToReplaceAction) {
String dataSource = ((RetrieveSegmentsToReplaceAction) taskAction).getDataSource();
return (RetType) injector.getInstance(SpecificSegmentsQuerySegmentWalker.class)

Copy link
Copy Markdown
Contributor

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?

@cryptoe
cryptoe merged commit 4a594bb into apache:master Dec 1, 2023
yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023
* 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]>
yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023
* 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]>
@kfaraz
kfaraz deleted the msq_retrieveSegmentsToReplace branch December 16, 2023 05:10
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants