Repository navigation
Conversation
testQueryTooMuchData only accepted the bundle-reservation capacity error, but which reservation runs out of space first depends on load thread timing. When the space left after the bundles that fit is smaller than the metadata reservation estimate, the partial metadata reservation fails first with "Unable to reserve partial metadata for segment[...]", which failed both assertions. Assert on the text shared by every virtual storage capacity error and include the actual message in the assertion failure. Add a unit test in SegmentLocalCacheManagerPartialAcquireTest covering the partial metadata CAPACITY_EXCEEDED path. Fixes apache#20550
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #20550.
Description
QueryVirtualStorageTest.testQueryTooMuchDataruns acount(*)over ~3.7MB of segments against a 1MiB virtual storage cache and expects the query to fail for lack of space. The query does fail as intended, but the test only accepted the bundle-reservation message (Unable to reserve bundle[...]). Which reservation runs out of space first depends on load thread timing, so the test occasionally saw a different, equally correctCAPACITY_EXCEEDEDerror and failed atQueryVirtualStorageTest.java:166.Why the error message varies
Native queries acquire segments with
AcquireMode.FULLin a loop inServerManager#getOrLoadSegmentReferences. EachacquireSegmentcall synchronously reserves the segment's partial metadata (SegmentLocalCacheManager#reservePartial, sized byvirtualStorageMetadataReservationEstimate, 2KiB in this test) and then hands the bundle downloads to the load threads, which reserve bundles concurrently.Unable to reserve bundle[...] for segment[...]; ensure enough disk space has been allocated to load all segments involved in the query.Unable to reserve partial metadata for segment[...]; ensure enough disk space has been allocated. That error is thrown from the acquire loop itself, so it is the one reported. It fails both of the old assertions, since it is not the bundle message and it does not end with "...to load all segments involved in the query".I confirmed this locally. Raising the test's metadata estimate to 120KiB makes the partial-metadata outcome happen on every run. Under that setting the unchanged test fails with
expected: <true> but was: <false>at line 166 (the same signature as the CI failure), and passes with this change. With the real 2KiB config the full class passes (4/4).Relaxed the assertion in
QueryVirtualStorageTesttestQueryTooMuchDatanow asserts onensure enough disk space has been allocated, the text shared by every virtual storage capacity error (partial metadata, bundle, and full segment load). It still fails if the query errors for an unrelated reason, such as a timeout or a download failure. The assertion now passest.getMessage()as its failure message. The CI failure only showedexpected: <true> but was: <false>, which is why the actual error had to be reproduced locally.Alternatives considered:
CAPACITY_EXCEEDEDcategory. Not viable: by the time the error reaches the broker's HTTP response it is reported as"category":"UNCATEGORIZED", so only the message text is available to the test.virtualStorageLoadThreads=1. Metadata is reserved on the query thread while bundles are reserved by load tasks, so this doesn't remove the race, and it would change the shared cluster thattestQueryPartialsrelies on for its load-count expectations.Added a unit test for the partial metadata capacity failure
SegmentLocalCacheManagerPartialAcquireTest#testFullAcquireFailsWhenPartialMetadataCannotBeReservedbuilds a manager whose only location is smaller than the metadata reservation estimate and checks thatacquireSegment(..., AcquireMode.FULL):CAPACITY_EXCEEDEDDruidException;Unable to reserve partial metadata for segment[...]message, including the shared "ensure enough disk space has been allocated" text the embedded test now relies on;No existing unit test covered this path. It is deterministic and runs in milliseconds, unlike the embedded race.
Release note
No user-facing changes. This PR only changes tests.
Key changed/added classes in this PR
QueryVirtualStorageTestSegmentLocalCacheManagerPartialAcquireTestThis PR has:
Generative AI usage
Generated by: Claude Opus 5.5