Fix option-4 empty-prefix LIST regression; add window-function LIST_ALL - #3265
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3265 +/- ##
=============================================
- Coverage 64.24% 51.10% -13.15%
+ Complexity 10398 8700 -1698
=============================================
Files 840 936 +96
Lines 71755 79921 +8166
Branches 8611 9579 +968
=============================================
- Hits 46099 40842 -5257
- Misses 23004 35684 +12680
- Partials 2652 3395 +743 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Two related changes: 1. NamedBlobPath.parseS3: collapse empty-string `prefix` to null. S3 clients can send `prefix=` with no value, which is semantically equivalent to omitting the prefix entirely. Pre-linkedin#3260 with option 3, `prefix=""` still completed under MAX_EXECUTION_TIME because the correlated-subquery plan streamed per-row probes. Under linkedin#3260 option 4, the same request executes LIST_WITH_PREFIX_SQL with `blob_name LIKE '%'`, which TiDB plans as Window-over-full-table- scan and can exceed the server kill on large containers. Routing the empty case to LIST_ALL_QUERY restores the bounded plan. 2. MySqlNamedBlobDb: gate LIST_ALL_QUERY behind the same listNamedBlobsSQLOption knob as LIST_WITH_PREFIX_SQL. Options 2/3 keep the existing INNER-JOIN + MAX-grouped subquery shape; option 4 adds a window-function variant matching the option-4 LIST_WITH_PREFIX_SQL plan shape and semantic (deleted_ts on OUTER select, hide-blob-if-latest-deleted). Operators flipping to option 4 get the same single-PK-scan-plus-streaming-window plan for both with-prefix and no-prefix LISTs, and a unified deleted-latest semantic across the two paths. Why this gap existed -------------------- linkedin#3260's integration matrix exercised non-empty prefixes only; the unbounded `LIKE '%'` case under option 4 was not validated. Reported via a downstream S3 correctness test failing testSuccessListObjectsEmptyPrefix after an option-4 canary deploy against a container with a substantial number of named blobs. Semantics --------- - Options 2/3 LIST_ALL_QUERY: unchanged. Inner deleted_ts filter preserved -> if latest version is soft-deleted, second-latest non-deleted surfaces. (Different from LIST_WITH_PREFIX_SQL options 2/3, but preserves pre-existing behavior.) - Option 4 LIST_ALL: window function with deleted_ts on OUTER select -> if latest version is soft-deleted, the blob is hidden entirely, matching LIST_WITH_PREFIX_SQL option 4. - Option 4 deployments inherit a behavior change on no-prefix LIST: latest-deleted blobs now hide instead of surfacing the second-latest. This unifies the semantic between with-prefix and no-prefix; option 2/3 deployments are unaffected. Compatibility ------------- - Option 4's window function requires MySQL 8.0+ or TiDB. Already enforced by config range and by the option-4 LIST_WITH_PREFIX_SQL shipped in linkedin#3260; no new compatibility constraint. - Default listNamedBlobsSQLOption remains 2. No behavior change for any deployment that has not opted into option 4. - The empty-prefix normalization is unconditional but only affects S3 callers, and only when they explicitly send `prefix=` empty. Non-S3 paths and S3 callers that omit prefix entirely are unchanged. Testing Done ------------ - ./gradlew :ambry-api:compileJava :ambry-named-mysql:compileJava :ambry-named-mysql:compileTestJava :ambry-named-mysql:compileIntTestJava -> BUILD SUCCESSFUL - ./gradlew :ambry-api:test --tests NamedBlobPathTest -> 10/10 pass including 3 new tests (empty-prefix-collapses-to-null, omitted-prefix-is-null, non-empty-prefix-preserved). - ./gradlew :ambry-named-mysql:test -> all unit tests pass. - Integration tests (intTest) not run locally; CI will exercise LIST_ALL via the existing MySqlNamedBlobDbListOperationIntegrationTest matrix for options 2/3/4. Durability risk: read-only LIST routing change. No write path, no schema, no atomicity, no callback, no resource cleanup. No durability concern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After NamedBlobPath.parseS3 normalizes empty-string prefix to null, the in-memory test impl was hit with null where it previously got "". TreeMap.tailMap(null) NPEs, and entry.getKey().startsWith(null) NPEs, breaking S3ListHandlerTest's empty-prefix and no-prefix-with-grouping cases. Mirror MySqlNamedBlobDb's LIST_ALL_QUERY path: when both prefix and pageToken are null, iterate the entire container; when prefix is null but pageToken is set, tailMap by pageToken only; skip the startsWith filter when prefix is null. Testing Done - ./gradlew :ambry-frontend:test --tests S3ListHandlerTest -> 7/7 pass - ./gradlew :ambry-frontend:intTest --tests S3IntegrationTest -> 2/2 pass locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4cd3fcc to
9d18396
Compare
| // listNamedBlobsSQLOption=4 the latter produces a Window-over-full-container-scan | ||
| // plan that can exceed MAX_EXECUTION_TIME on large containers, while LIST_ALL_QUERY | ||
| // is purpose-built for the unbounded case. | ||
| if (blobNamePrefix != null && blobNamePrefix.isEmpty()) { |
There was a problem hiding this comment.
do you also want to trim before checking empty?
There was a problem hiding this comment.
considered the trim it but opted not to.
AWS S3 semantics treat prefix=" " as a real filter on names starting with a space rather than as equivalent to no prefix,
and trimming would silently change behavior for any client that legitimately wants to list whitespace-leading blobs.
it also seems like in current Ambry System we allow one empty space but not 2 consecutive spaces
https://github.com/linkedin/ambry/blob/755e2c5450a22f891671918909b733860e77d0cf/am[…]/java/com/github/ambry/frontend/FrontendRestRequestService.java
…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>
…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>
…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>
…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>
…rge containers (#3272) * Close LIST_ALL_QUERY (null-prefix) and S3-handler empty-prefix coverage gaps After #3265 fixed the empty-prefix LIST regression on option 4, two test-coverage gaps remained that #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 #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 #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> * Fix option-4 LIST_ALL (empty-prefix S3 LIST) timeout/HTTP 500 on large containers An empty-prefix S3 ListObjects collapses to a null prefix (#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> --------- Co-authored-by: Zicheng Liu <8845012+zichengl@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Two related changes for the option-4 (window-function) LIST path introduced in #3260:
NamedBlobPath.parseS3: collapse empty-stringprefixtonull. S3 clients can sendprefix=with no value, which is semantically equivalent to omitting the prefix entirely. Routing the empty case toLIST_ALL_QUERYavoids triggeringLIST_WITH_PREFIX_SQLwithblob_name LIKE '%', which under option 4 produces a Window-over-full-container-scan plan that can exceedMAX_EXECUTION_TIMEon large containers.MySqlNamedBlobDb: gateLIST_ALL_QUERYbehind the samelistNamedBlobsSQLOptionknob asLIST_WITH_PREFIX_SQL. Options 2/3 keep the existing INNER-JOIN + MAX-grouped subquery shape; option 4 adds a window-function variant matching the option-4 with-prefix plan shape and semantic.Default
listNamedBlobsSQLOptionremains 2. Option 2 and 3 deployments are byte-identical to master.Motivation
After deploying option 4 (
#3260shipped in0.5.163) downstream, a load/correctness suite failedS3ListTests.testSuccessListObjectsEmptyPrefixagainst a container with a substantial number of named blobs. All other S3 LIST tests passed including the prefix-filtering tests that directly exercise option 4's new SQL — the only failure was the empty-prefix case.Tracing through the OSS code path: S3
ListObjectswith explicitprefix=""arrives atNamedBlobPath.parseS3asblobNamePrefix = ""(notnull), then throughNamedBlobListHandler.listRecursivelyInternalintoMySqlNamedBlobDb.run_list_v2:""is notnull, so the empty case routes toLIST_WITH_PREFIX_SQL. Withprefix="", the V4 binder produces parameters("%", ""), which under option 4 plans as:(account_id, container_id)prefixWindow(MAX(version) OVER (PARTITION BY blob_name))over the full resultSelection+ORDER BY blob_name LIMIT NOn a large enough container this exceeds a typical 5s
MAX_EXECUTION_TIMEserver kill, returning 500 to the S3 SDK after 3 retries. Pre-#3260 with option 3, the same empty-prefix request also routed toLIST_WITH_PREFIX_SQLbut used the correlated-subquery shape, whose per-row probe pattern apparently completed within the kill threshold on the same dataset.The OSS integration matrix in #3260 (
MySqlNamedBlobDbListOperationIntegrationTest) only exercises non-empty prefixes, so this unbounded-scan failure mode of option 4 was not caught upstream.What this PR changes
NamedBlobPath.parseS3S3-only. Non-S3 callers (which use
isListRequest = blobNamePrefix != nullto detect a list request) are unchanged so they don't change list-request detection semantics.MySqlNamedBlobDb.LIST_ALL_QUERY→LIST_ALL_SQLConvert from
static finalconstant to an instance field initialized viagetListAllSQLStatement(config), mirroring the existingLIST_WITH_PREFIX_SQLpattern:MAX(version) OVER (PARTITION BY blob_name)over single PK range scan,deleted_tson OUTER selectFor option 2/3 deployments: byte-identical SQL to master. For option 4 deployments: get the window-function plan for both with-prefix and no-prefix paths, and a unified hide-latest-deleted semantic across the two paths.
InMemNamedBlobDbHandle
nullprefix to matchMySqlNamedBlobDb'sLIST_ALL_QUERYpath. Previously the in-memory test impl assumed non-null prefix and would NPE inTreeMap.tailMap(null)andentry.getKey().startsWith(null). With parseS3 now normalizing empty to null, the in-memory impl needs the same null-tolerant branching.Compatibility
listNamedBlobsSQLOptionremains 2.MySqlNamedBlobDbConfigfor option 4 LIST_WITH_PREFIX; no new constraint here.prefix=empty.Testing Done
./gradlew :ambry-api:compileJava :ambry-named-mysql:compileJava :ambry-named-mysql:compileTestJava :ambry-named-mysql:compileIntTestJava→ BUILD SUCCESSFUL../gradlew :ambry-api:test --tests com.github.ambry.frontend.NamedBlobPathTest→ 10/10 pass, including 3 new cases:testParseS3EmptyPrefixCollapsesToNulltestParseS3OmittedPrefixIsNulltestParseS3NonEmptyPrefixPreserved./gradlew :ambry-named-mysql:test→ all unit tests pass../gradlew :ambry-frontend:test --tests S3ListHandlerTest→ 7/7 pass../gradlew :ambry-frontend:intTest --tests S3IntegrationTest→ 2/2 pass locally.MySqlNamedBlobDbListOperationIntegrationTestalready parameterizes overlistSqlOption ∈ {2, 3, 4}and will exercise the newLIST_ALL_SQLvariants on CI.Durability risk
Read-only LIST routing change. No write path, no schema, no atomicity, no callback semantics, no resource cleanup. No durability concern.