Skip to content
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

Batch segment retrieval from the metadata store #15305

Conversation

abhishekrb19
Copy link
Contributor

@abhishekrb19 abhishekrb19 commented Nov 2, 2023

Used segments retrieval fails when there are too many intervals (about 120 with Derby). Same thing can happen with multiple intervals as with unused segments.

Previously, the intervals weren't capped in the SQL query and is bloated by the fact that we add grouped OR clause per interval to handle the eternity interval here. This PR splits up the SELECT query into multiple batches, with 100 intervals/batch. This is similar to the batching strategy with a cap on max number of segments announced at once.

org.skife.jdbi.v2.exceptions.CallbackFailedException: org.skife.jdbi.v2.exceptions.UnableToCreateStatementException: 

java.sql.SQLSyntaxErrorException: Statement too complex. Try rewriting the query to remove complexity. Eliminating many duplicate expressions or breaking up the query and storing interim results in a temporary table can often help resolve this error. 
[statement:"SELECT payload FROM foo WHERE used = :used AND dataSource = :dataSource AND ((start < :end0 AND \"end\" > :start0) OR (start = '-146136543-09-08T08:23:32.096Z' AND "end" != '146140482-04-24T15:36:27.903Z' AND "end" > :start0) OR (start != '-146136543-09-08T08:23:32.096Z' AND "end" = '146140482-04-24T15:36:27.903Z' AND start < :end0) OR (start < :end1 AND \"end\" > :start1) OR (start = '-146136543-09-08T08:23:32.096Z' AND "end" != '146140482-04-24T15:36:27.903Z' AND "end" > :start1) OR (start != '-146136543-09-08T08:23:32.096Z' AND "end" = '146140482-04-24T15:36:27.903Z' AND start < :end1) OR (start < :end2 AND \"end\"

Core changes:

  • Segment retrieval is batched at the storage layer by intervals. It's capped at 100 intervals per batch.
  • With multiple batches, the iterators are concatenated together and returned to the caller.
  • Add a combination of unit tests that test the different scenarios with limit in, at and out of range scenarios with used and unused segments retrieval.

A fix localized to kill tasks that originally exposed this bug is available here - #15306. We'd still separately want this change in the server as the issue can happen more broadly.

This PR has:

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

… are retrieved.

- This is a failing test case that needs to be ignored.
@abhishekrb19 abhishekrb19 marked this pull request as draft November 2, 2023 02:35
@abhishekrb19 abhishekrb19 marked this pull request as ready for review November 2, 2023 06:35
@abhishekrb19 abhishekrb19 changed the title Used segments retrieval fails when there are too many intervals Batch segment retrieval from the metadata store Nov 3, 2023
@zachjsh zachjsh self-requested a review November 3, 2023 18:25
Copy link
Contributor

@zachjsh zachjsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abhishekrb19 abhishekrb19 merged commit 2136dc3 into apache:master Nov 6, 2023
11 checks passed
@abhishekrb19 abhishekrb19 deleted the retrieve_used_segments_too_many_intervals_test branch November 6, 2023 19:30
CaseyPan pushed a commit to CaseyPan/druid that referenced this pull request Nov 17, 2023
* Add a unit test that fails when used segments with too many intervals are retrieved.

- This is a failing test case that needs to be ignored.

* Batch the intervals (use 100 as it's consistent with batching in other places).

* move the filtering inside the batch

* Account for limit cross the batch splits.

* Adjustments

* Fixup and add tests

* small refactor

* add more tests.

* remove wrapper.

* Minor edits

* assert out of range
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants