Fix option-4 LIST_ALL (empty-prefix S3 LIST) timeout / HTTP 500 on large containers#3272
Open
zichengl wants to merge 2 commits into
Open
Fix option-4 LIST_ALL (empty-prefix S3 LIST) timeout / HTTP 500 on large containers#3272zichengl wants to merge 2 commits into
zichengl wants to merge 2 commits into
Conversation
127a871 to
0e1c73c
Compare
…ge gaps After linkedin#3265 fixed the empty-prefix LIST regression on option 4, two test-coverage gaps remained that linkedin#3265 didn't close: 1. MySqlNamedBlobDbListOperationIntegrationTest exercised list() only with non-null prefixes, so LIST_ALL_QUERY (and the new option-4 LIST_ALL_SQL window-function variant) had no parametrized integration coverage. The empty-prefix LIST failure that motivated linkedin#3265 specifically traversed this path (S3 SDK sends prefix= empty -> parseS3 collapses to null -> list() with null prefix -> LIST_ALL under option 4), and the int-test matrix would not have caught a regression in that branch. 2. S3IntegrationTest had no end-to-end test that explicitly emits `?prefix=` (parameter present, value empty) -- the exact request shape the AWS S3 SDK uses when ListObjectsRequest is built with an empty or unset prefix. Without it, a future refactor of S3ListHandler or NamedBlobPath.parseS3 that drops the empty->null collapse would slip past CI. This commit adds both: (a) ambry-named-mysql/src/integration-test: - testListNamedBlobsWithNullPrefix -- option-agnostic basic test that PUTs 5 blobs and asserts list(account, container, null, null, null) returns all 5 in blob_name order. Runs against options 2/3/4 via the existing @parameterized matrix. - testListAllNullPrefixHidesDeletedLatestUnderOption4 -- option-4 only via Assume.assumeTrue; mirrors the invariant testListHidesBlobWhenLatestVersionIsExpired added in linkedin#3260 but for the null-prefix path. Verifies that under option 4 a blob whose latest version is TTL-expired is hidden entirely from LIST_ALL_SQL (the deleted_ts-on-outer-SELECT contract). Options 2/3 have the opposite legacy semantic by design and are excluded. (b) ambry-frontend/src/integration-test: - s3ListEmptyPrefixTest -- exercises GET /s3/{account}/{container}?prefix= end-to-end through HTTP -> Netty -> S3ListHandler -> NamedBlobPath.parseS3 -> NamedBlobListHandler -> NamedBlobDb#list. Asserts 200 OK for both v1 (no list-type) and v2 (list-type=2) shapes. The named-blob DB in this integration test is InMemNamedBlobDbFactory, so this catches regressions in the S3-handler routing layer (empty-prefix collapse, parseS3 logic) but not SQL-side regressions -- those are covered by (a) against a real MySQL backend in :ambry-named-mysql:intTest. Full stitched coverage (S3 handler -> real MySQL with option 4 on a real container) is left to a follow-up that refactors S3IntegrationTest's buildFrontendVProps to accept a configurable NamedBlobDbFactory, allowing a sibling test class to swap in the MySQL-backed factory. That refactor wasn't bundled here to keep this diff small and review-focused on the specific gaps. Testing Done ------------ - ./gradlew :ambry-named-mysql:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-frontend:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-named-mysql:intTest --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListNamedBlobs WithNullPrefix*' --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListAllNullPrefix HidesDeletedLatestUnderOption4*' -> 12 runs (6 per method x 2 methods), 8 passed, 4 skipped. The 4 skipped are the option-4-only invariant on options 2/3 via Assume.assumeTrue, as intended. - ./gradlew :ambry-frontend:intTest --tests '*S3IntegrationTest.s3ListEmptyPrefixTest' -> 1 passed. - Full :ambry-named-mysql:intTest matrix runs against a local MySQL 8.0 container; passes 30/30 baseline + 12 new = 42 total with 8 added passes + 4 added Assume-skips. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0e1c73c to
ae09e1a
Compare
…e containers An empty-prefix S3 ListObjects collapses to a null prefix (linkedin#3265) and routes to LIST_ALL. Under listNamedBlobsSqlOption=4, LIST_ALL used a window-function query (MAX(version) OVER (PARTITION BY blob_name)) whose derived table is materialized in full before the outer LIMIT -- it scans the entire container and cannot early-terminate. On large containers it ran past a short socket timeout, threw "Communications link failure", and returned HTTP 500. Because the empty-prefix path is the only way an S3 client reaches LIST_ALL, this blocks the option-4 rollout: any LIST with an empty prefix on a large container would 500. Fix: - Replace the option-4 LIST_ALL window query with a correlated-subquery form (the LIST_WITH_PREFIX option-3 shape, minus the prefix predicate). The outer scans candidates in PRIMARY KEY (blob_name) order and keeps only version = MAX(version); deleted_ts sits on the outer candidate, so a soft-deleted latest version hides the blob entirely (no older-version leak) -- preserving option-4 semantics. With the PK already blob_name-ordered, ORDER BY blob_name LIMIT N early-terminates after N matches instead of materializing the whole container. Options 2/3 LIST_ALL are unchanged. - run_list_v2 now switches the null-prefix binder by option (2/3 -> 5 params, 4 -> new constructListAllQueryV4 -> 7 params) to match the statement. - Add a default-off list query-timeout knob (mysql.named.blob.list.query.timeout.seconds) applied via Statement.setQueryTimeout, so a residual slow LIST surfaces a clean SQLException instead of tearing the connection (500). No-op for existing fabrics. Testing Done: - ./gradlew :ambry-named-mysql:intTest --tests "*MySqlNamedBlobDbListOperationIntegrationTest" against MySQL 8.0: 54 tests, 46 passed, 0 failed, 8 skipped (option-4-only invariants Assume-skip on options 2/3). Includes new testListAllNullPrefixPagination (options 2/3/4) and testListAllNullPrefixReturnsLatestVersionUnderOption4, plus the existing testListAllNullPrefixHidesDeletedLatestUnderOption4 regression guard (green under the new option-4 LIST_ALL). - ./gradlew :ambry-frontend:intTest --tests "*S3MySqlNamedBlobListIntegrationTest" --tests "*S3IntegrationTest" against MySQL 8.0: 4 passed, 0 failed. The new S3MySqlNamedBlobListIntegrationTest exercises the empty-prefix LIST end-to-end (S3 SDK -> Netty -> S3ListHandler -> parseS3 -> NamedBlobListHandler -> MySqlNamedBlobDb.list null-prefix -> option-4 LIST_ALL) against real MySQL and asserts 200 -- the stitched coverage previously deferred; existing S3IntegrationTest still passes after the buildFrontendVProps refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ae09e1a to
d986855
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3272 +/- ##
=============================================
- Coverage 64.24% 50.82% -13.43%
+ Complexity 10398 8687 -1711
=============================================
Files 840 938 +98
Lines 71755 80339 +8584
Branches 8611 9666 +1055
=============================================
- Hits 46099 40830 -5269
- Misses 23004 36115 +13111
- Partials 2652 3394 +742 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
Under
listNamedBlobsSqlOption=4, an empty-prefix S3ListObjects(which#3265collapses to a null prefix and routes toLIST_ALL) used a window-function query —MAX(version) OVER (PARTITION BY blob_name)inside a derived table. MySQL materializes that derived table in full before the outerLIMIT, so it scans the entire container and cannot early-terminate. On large containers it runs past a short socket timeout, throwsCommunications link failure, and returns HTTP 500.Because the empty-prefix path is the only way an S3 client reaches
LIST_ALL, this blocks rolling option 4 to any fabric with large containers — an empty-prefix LIST there would 500.Fix
LIST_ALLwindow query with a correlated-subquery form (theLIST_WITH_PREFIXoption-3 shape, minus the prefix predicate). The outer scans candidates in PRIMARY KEY (blob_name) order and keeps onlyversion = MAX(version); thedeleted_tspredicate sits on the outer candidate, so a soft-deleted latest version hides the blob entirely (no older-version leak) — preserving option-4 semantics. With the PK alreadyblob_name-ordered,ORDER BY blob_name LIMIT Nearly-terminates after N matches instead of materializing the whole container. Options 2/3LIST_ALLare unchanged.run_list_v2now selects the null-prefix binder by option (2/3 → 5 params, 4 → newconstructListAllQueryV4→ 7 params) so the statement and binder stay in lockstep.mysql.named.blob.list.query.timeout.secondsknob applied viaStatement.setQueryTimeout, so a residual slow LIST surfaces a cleanSQLExceptionrather than tearing the connection into a 500. No-op for existing deployments.This PR also carries the earlier follow-up commit that added null-prefix
LIST_ALLtest coverage (the regression guards this fix relies on).Testing Done
Against MySQL 8.0:
./gradlew :ambry-named-mysql:intTest --tests "*MySqlNamedBlobDbListOperationIntegrationTest"→ 54 tests, 46 passed, 0 failed, 8 skipped (the 8 are option-4-only invariants thatAssume-skip on options 2/3). New:testListAllNullPrefixPagination(options 2/3/4) andtestListAllNullPrefixReturnsLatestVersionUnderOption4; the existingtestListAllNullPrefixHidesDeletedLatestUnderOption4stays green under the new option-4LIST_ALL../gradlew :ambry-frontend:intTest --tests "*S3MySqlNamedBlobListIntegrationTest" --tests "*S3IntegrationTest"→ 4 passed, 0 failed. The newS3MySqlNamedBlobListIntegrationTestexercises the empty-prefix LIST end-to-end (S3 SDK → Netty →S3ListHandler→parseS3→NamedBlobListHandler→MySqlNamedBlobDb.listnull-prefix → option-4LIST_ALL) against real MySQL and asserts 200 — the stitched coverage previously deferred. ExistingS3IntegrationTeststill passes after thebuildFrontendVPropsrefactor.Risk / rollout
LIST_ALLkeeps the same hide-latest-deleted semantic, just via a streaming plan.🤖 Generated with Claude Code