diff --git a/ambry-api/src/main/java/com/github/ambry/config/MySqlNamedBlobDbConfig.java b/ambry-api/src/main/java/com/github/ambry/config/MySqlNamedBlobDbConfig.java index 6fe0570152..e10fc46b02 100644 --- a/ambry-api/src/main/java/com/github/ambry/config/MySqlNamedBlobDbConfig.java +++ b/ambry-api/src/main/java/com/github/ambry/config/MySqlNamedBlobDbConfig.java @@ -32,6 +32,7 @@ public class MySqlNamedBlobDbConfig { public static final String LIST_MAX_RESULTS = PREFIX + "list.max.results"; public static final String QUERY_STALE_DATA_MAX_RESULTS = PREFIX + "query.stale.data.max.results"; public static final String STALE_DATA_RETENTION_DAYS = PREFIX + "stale.data.retention.days"; + public static final String LIST_QUERY_TIMEOUT_SECONDS = PREFIX + "list.query.timeout.seconds"; public static final String TRANSACTION_ISOLATION_LEVEL = PREFIX + "transaction.isolation.level"; public static final String LIST_NAMED_BLOBS_SQL_OPTION = "list.named.blobs.sql.option"; public static final String ENABLE_HARD_DELETE = PREFIX + "enable.hard.delete"; @@ -117,6 +118,20 @@ public class MySqlNamedBlobDbConfig { @Default("5") public final int staleDataRetentionDays; + /** + * Per-statement timeout (in seconds) applied to LIST queries via {@link java.sql.Statement#setQueryTimeout(int)}. + * Guards against a LIST scanning an unexpectedly large container: when the budget is exceeded the JDBC driver + * issues a clean cancel and the server throws a {@link java.sql.SQLException} (e.g. MySQLTimeoutException), + * which surfaces as an ordinary error rather than tearing the socket ("Communications link failure" -> HTTP 500). + * + *

Default 0 disables the timeout, making this a no-op for existing deployments. Operators on fabrics + * where a network/socket timeout is shorter than the server statement kill should set this just below that + * socket timeout so a slow LIST fails fast and cleanly instead of poisoning the connection. + */ + @Config(LIST_QUERY_TIMEOUT_SECONDS) + @Default("0") + public final int listQueryTimeoutSeconds; + /** * Transaction isolation level to be set on DB Connection. When nothing is set, default MySQL DB transaction level * (REPEATABLE_READ) will take effect. @@ -164,6 +179,8 @@ public MySqlNamedBlobDbConfig(VerifiableProperties verifiableProperties) { verifiableProperties.getIntInRange(QUERY_STALE_DATA_MAX_RESULTS, 1000, 1, Integer.MAX_VALUE); this.staleDataRetentionDays = verifiableProperties.getIntInRange(STALE_DATA_RETENTION_DAYS, 5, 1, Integer.MAX_VALUE); + this.listQueryTimeoutSeconds = + verifiableProperties.getIntInRange(LIST_QUERY_TIMEOUT_SECONDS, 0, 0, Integer.MAX_VALUE); this.transactionIsolationLevel = verifiableProperties.getEnum(TRANSACTION_ISOLATION_LEVEL, TransactionIsolationLevel.class, TransactionIsolationLevel.TRANSACTION_NONE); diff --git a/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java index 678caadbf1..56cce9a1d7 100644 --- a/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java +++ b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java @@ -327,15 +327,90 @@ private void doListBlob(String account, String container, String key) throws Exc assertEquals("Mismatch in encoding type", "url", listBucketResult.getEncodingType()); } + /** + * Reproduces the request shape that the AWS S3 SDK emits when the caller sets an unset/empty + * prefix on ListObjectsRequest: the query string contains {@code prefix=} (parameter present, + * value empty) rather than omitting the parameter entirely. This is the exact shape that + * caused the original empty-prefix regression on option 4 (LIST_WITH_PREFIX_SQL evaluated + * {@code blob_name LIKE '%'} over a full container scan, timing out at MAX_EXECUTION_TIME); + * fixed in linkedin/ambry#3265 by collapsing empty {@code prefix} to {@code null} in + * {@link NamedBlobPath#parseS3}. + * + * This test asserts the request path returns 200 OK end-to-end through the S3 handler stack + * (HTTP → Netty → {@code S3ListHandler} → {@code NamedBlobPath.parseS3} → {@code + * NamedBlobListHandler} → {@code NamedBlobDb#list}). The named-blob DB in this integration + * test is {@code InMemNamedBlobDbFactory}, so this test specifically catches regressions in + * the S3-handler routing layer (empty-prefix collapse, parseS3 logic) — not SQL-side + * regressions, which are covered by + * {@code MySqlNamedBlobDbListOperationIntegrationTest#testListNamedBlobsWithNullPrefix} + * against a real MySQL backend. + */ + @Test + public void s3ListEmptyPrefixTest() throws Exception { + Container container = ACCOUNT.getAllContainers().iterator().next(); + String account = ACCOUNT.getName(); + String containerName = container.getName(); + + // Seed a couple of blobs so the LIST has something to return — the test focuses on the + // request shape and routing, not on the content of the response. + String[] keys = new String[]{"empty_prefix_seed_a", "empty_prefix_seed_b"}; + int contentSize = 64; + for (String key : keys) { + byte[] content = TestUtils.getRandomBytes(contentSize); + doPutBlob(account, containerName, key, contentSize, content); + } + + // V1 LIST: GET /s3/{account}/{container}?prefix= (explicit empty value) + String uriV1 = String.format("/s3/%s/%s?prefix=", account, containerName); + HttpHeaders headers = new DefaultHttpHeaders(); + FullHttpRequest reqV1 = buildRequest(HttpMethod.GET, uriV1, headers, null); + NettyClient.ResponseParts partsV1 = nettyClient.sendRequest(reqV1, null, null).get(); + HttpResponse respV1 = getHttpResponse(partsV1); + assertEquals("LIST v1 with explicit empty prefix should return 200 OK end-to-end through " + + "the S3 handler stack; regression in parseS3 empty-prefix collapse would surface here", + HttpResponseStatus.OK, respV1.status()); + + // V2 LIST: GET /s3/{account}/{container}?prefix=&list-type=2 + String uriV2 = String.format("/s3/%s/%s?prefix=&list-type=2", account, containerName); + FullHttpRequest reqV2 = buildRequest(HttpMethod.GET, uriV2, new DefaultHttpHeaders(), null); + NettyClient.ResponseParts partsV2 = nettyClient.sendRequest(reqV2, null, null).get(); + HttpResponse respV2 = getHttpResponse(partsV2); + assertEquals("LIST v2 with explicit empty prefix should return 200 OK end-to-end", + HttpResponseStatus.OK, respV2.status()); + + // Cleanup + for (String key : keys) { + String deleteUri = String.format("/s3/%s/%s/%s", account, containerName, key); + FullHttpRequest delReq = buildRequest(HttpMethod.DELETE, deleteUri, new DefaultHttpHeaders(), null); + nettyClient.sendRequest(delReq, null, null).get(); + } + } + /** * Builds properties required to start a {@link RestServer} as an Ambry frontend server. * @param trustStoreFile the trust store file to add certificates to for SSL testing. * @param account {@link Account} for which quota needs to be specified. * @return a {@link VerifiableProperties} with the parameters for an Ambry frontend server. */ - private static VerifiableProperties buildFrontendVPropsForQuota(File trustStoreFile, Account account) + static VerifiableProperties buildFrontendVPropsForQuota(File trustStoreFile, Account account) throws IOException, GeneralSecurityException { - Properties properties = buildFrontendVProps(trustStoreFile); + return buildFrontendVPropsForQuota(trustStoreFile, account, "com.github.ambry.commons.InMemNamedBlobDbFactory", + null); + } + + /** + * Builds quota-enabled frontend properties, letting the caller pick the named-blob DB factory (e.g. the + * MySQL-backed factory) and supply extra properties (e.g. the dbInfo and LIST SQL option). Reused by + * sibling integration tests that exercise the S3 stack against a real backend. + * @param trustStoreFile the trust store file to add certificates to for SSL testing. + * @param account {@link Account} for which quota needs to be specified. + * @param namedBlobDbFactory the fully-qualified {@link com.github.ambry.named.NamedBlobDbFactory} class name. + * @param extraProps additional properties to layer on top (may be null). + * @return a {@link VerifiableProperties} with the parameters for an Ambry frontend server. + */ + static VerifiableProperties buildFrontendVPropsForQuota(File trustStoreFile, Account account, + String namedBlobDbFactory, Properties extraProps) throws IOException, GeneralSecurityException { + Properties properties = buildFrontendVProps(trustStoreFile, namedBlobDbFactory, extraProps); JSONObject cuResourceQuotaJson = new JSONObject(); JSONObject quotaJson = new JSONObject(); quotaJson.put("rcu", 10737418240L); @@ -354,7 +429,18 @@ private static VerifiableProperties buildFrontendVPropsForQuota(File trustStoreF * @param trustStoreFile the trust store file to add certificates to for SSL testing. * @return a {@link Properties} with the parameters for an Ambry frontend server. */ - private static Properties buildFrontendVProps(File trustStoreFile) + static Properties buildFrontendVProps(File trustStoreFile) throws IOException, GeneralSecurityException { + return buildFrontendVProps(trustStoreFile, "com.github.ambry.commons.InMemNamedBlobDbFactory", null); + } + + /** + * Builds frontend properties with a caller-selected named-blob DB factory and optional extra properties. + * @param trustStoreFile the trust store file to add certificates to for SSL testing. + * @param namedBlobDbFactory the fully-qualified {@link com.github.ambry.named.NamedBlobDbFactory} class name. + * @param extraProps additional properties to layer on top (may be null). + * @return a {@link Properties} with the parameters for an Ambry frontend server. + */ + static Properties buildFrontendVProps(File trustStoreFile, String namedBlobDbFactory, Properties extraProps) throws IOException, GeneralSecurityException { Properties properties = new Properties(); properties.put("rest.server.rest.request.service.factory", @@ -377,8 +463,11 @@ private static Properties buildFrontendVProps(File trustStoreFile) properties.setProperty("clustermap.datacenter.name", DATA_CENTER_NAME); properties.setProperty("clustermap.host.name", HOST_NAME); properties.setProperty(FrontendConfig.ENABLE_UNDELETE, Boolean.toString(true)); - properties.setProperty(FrontendConfig.NAMED_BLOB_DB_FACTORY, "com.github.ambry.commons.InMemNamedBlobDbFactory"); + properties.setProperty(FrontendConfig.NAMED_BLOB_DB_FACTORY, namedBlobDbFactory); properties.setProperty(MySqlNamedBlobDbConfig.LIST_MAX_RESULTS, String.valueOf(NAMED_BLOB_LIST_RESULT_MAX)); + if (extraProps != null) { + properties.putAll(extraProps); + } return properties; } } diff --git a/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3MySqlNamedBlobListIntegrationTest.java b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3MySqlNamedBlobListIntegrationTest.java new file mode 100644 index 0000000000..f2c94abd4f --- /dev/null +++ b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3MySqlNamedBlobListIntegrationTest.java @@ -0,0 +1,212 @@ +/** + * Copyright 2026 LinkedIn Corp. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + */ +package com.github.ambry.frontend; + +import com.github.ambry.account.Account; +import com.github.ambry.account.Container; +import com.github.ambry.account.InMemAccountService; +import com.github.ambry.account.InMemAccountServiceFactory; +import com.github.ambry.clustermap.MockClusterMap; +import com.github.ambry.commons.LoggingNotificationSystem; +import com.github.ambry.commons.SSLFactory; +import com.github.ambry.commons.TestSSLUtils; +import com.github.ambry.config.FrontendConfig; +import com.github.ambry.config.MySqlNamedBlobDbConfig; +import com.github.ambry.config.SSLConfig; +import com.github.ambry.config.VerifiableProperties; +import com.github.ambry.quota.QuotaResourceType; +import com.github.ambry.rest.NettyClient; +import com.github.ambry.rest.RestServer; +import com.github.ambry.utils.TestUtils; +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.HttpHeaders; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpResponse; +import io.netty.handler.codec.http.HttpResponseStatus; +import java.io.File; +import java.nio.ByteBuffer; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Collections; +import java.util.Properties; +import org.junit.AfterClass; +import org.junit.Assume; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import static com.github.ambry.rest.RestUtils.Headers.*; +import static com.github.ambry.rest.RestUtils.*; +import static org.junit.Assert.*; + + +/** + * Integration test that exercises the S3 empty-prefix LIST path end-to-end against a REAL MySQL backend under + * {@code listNamedBlobsSqlOption=4}: HTTP -> Netty -> S3ListHandler -> NamedBlobPath.parseS3 (empty prefix collapses + * to null) -> NamedBlobListHandler -> {@link com.github.ambry.named.MySqlNamedBlobDb#list} -> LIST_ALL (option 4). + * + *

This is the stitched coverage that {@code S3IntegrationTest#s3ListEmptyPrefixTest} could not provide (it uses + * {@code InMemNamedBlobDbFactory}, so it never runs the SQL), and that the SQL-only int test + * {@code MySqlNamedBlobDbListOperationIntegrationTest} could not provide (it never goes through the S3 handler). It is + * the only layer that runs the exact path that returned HTTP 500 on large containers when LIST_ALL option 4 used a + * window-function plan that materialized the whole container. + * + *

The CI {@code int-test} job provisions {@code localhost/AmbryNamedBlobs} (NamedBlobsSchema) and the {@code travis} + * user, so this test runs there. When no such MySQL is reachable (e.g. local dev), it is skipped via {@link Assume}. + */ +public class S3MySqlNamedBlobListIntegrationTest extends FrontendIntegrationTestBase { + // dbInfo "datacenter" MUST match clustermap.datacenter.name that S3IntegrationTest.buildFrontendVProps sets. + private static final String DATA_CENTER_NAME = "localDc"; + private static final String NAMED_BLOB_DB_URL = "jdbc:mysql://localhost/AmbryNamedBlobs?serverTimezone=UTC"; + private static final String NAMED_BLOB_DB_USER = "travis"; + private static final String NAMED_BLOB_DB_PASSWORD = ""; + private static final String DB_INFO = + "[{\"url\":\"" + NAMED_BLOB_DB_URL + "\",\"datacenter\":\"" + DATA_CENTER_NAME + + "\",\"isWriteable\":\"true\",\"username\":\"" + NAMED_BLOB_DB_USER + "\",\"password\":\"" + + NAMED_BLOB_DB_PASSWORD + "\",\"sslMode\":\"NONE\"}]"; + + private static final MockClusterMap CLUSTER_MAP; + private static final VerifiableProperties FRONTEND_VERIFIABLE_PROPS; + private static final VerifiableProperties SSL_CLIENT_VERIFIABLE_PROPS; + private static final FrontendConfig FRONTEND_CONFIG; + // InMemAccountServiceFactory returns a (returnOnlyUnknown, notifyConsumers)-keyed singleton, so this is the SAME + // instance the frontend's account service resolves against -> the account created here is visible to MySqlNamedBlobDb. + private static final InMemAccountService ACCOUNT_SERVICE = + new InMemAccountServiceFactory(false, true).getAccountService(); + private static final Account ACCOUNT; + private static RestServer ambryRestServer = null; + private static NettyClient plaintextNettyClient = null; + private static NettyClient sslNettyClient = null; + private static boolean mysqlAvailable = false; + + static { + try { + CLUSTER_MAP = new MockClusterMap(); + File trustStoreFile = File.createTempFile("truststore", ".jks"); + trustStoreFile.deleteOnExit(); + SSL_CLIENT_VERIFIABLE_PROPS = TestSSLUtils.createSslProps("", SSLFactory.Mode.CLIENT, trustStoreFile, "client"); + ACCOUNT_SERVICE.clear(); + ACCOUNT_SERVICE.updateAccounts(Collections.singletonList(InMemAccountService.UNKNOWN_ACCOUNT)); + ACCOUNT = ACCOUNT_SERVICE.createAndAddRandomAccount(QuotaResourceType.ACCOUNT); + Properties mysqlProps = new Properties(); + mysqlProps.setProperty(MySqlNamedBlobDbConfig.DB_INFO, DB_INFO); + // Exercise the option-4 LIST_ALL path end-to-end (the empty-prefix -> null-prefix LIST that 500'd on large containers). + mysqlProps.setProperty(MySqlNamedBlobDbConfig.LIST_NAMED_BLOBS_SQL_OPTION, + Integer.toString(MySqlNamedBlobDbConfig.MAX_LIST_NAMED_BLOBS_SQL_OPTION)); + FRONTEND_VERIFIABLE_PROPS = S3IntegrationTest.buildFrontendVPropsForQuota(trustStoreFile, ACCOUNT, + "com.github.ambry.named.MySqlNamedBlobDbFactory", mysqlProps); + FRONTEND_CONFIG = new FrontendConfig(FRONTEND_VERIFIABLE_PROPS); + } catch (Throwable t) { + throw new IllegalStateException(t); + } + } + + public S3MySqlNamedBlobListIntegrationTest() { + super(FRONTEND_CONFIG, sslNettyClient); + } + + @BeforeClass + public static void setup() throws Exception { + // Skip cleanly when no int-test MySQL is reachable (e.g. local dev without it). CI's int-test job provisions + // localhost/AmbryNamedBlobs with the NamedBlobsSchema, so the test runs there. + try (Connection ignored = DriverManager.getConnection(NAMED_BLOB_DB_URL, NAMED_BLOB_DB_USER, + NAMED_BLOB_DB_PASSWORD)) { + mysqlAvailable = true; + } catch (SQLException e) { + Assume.assumeNoException("MySQL (localhost/AmbryNamedBlobs) not reachable; skipping MySQL-backed S3 LIST test", + e); + } + ambryRestServer = new RestServer(FRONTEND_VERIFIABLE_PROPS, CLUSTER_MAP, new LoggingNotificationSystem(), + SSLFactory.getNewInstance(new SSLConfig(FRONTEND_VERIFIABLE_PROPS))); + ambryRestServer.start(); + plaintextNettyClient = new NettyClient("localhost", PLAINTEXT_SERVER_PORT, null); + sslNettyClient = new NettyClient("localhost", SSL_SERVER_PORT, + SSLFactory.getNewInstance(new SSLConfig(SSL_CLIENT_VERIFIABLE_PROPS))); + } + + @AfterClass + public static void teardown() { + if (plaintextNettyClient != null) { + plaintextNettyClient.close(); + } + if (sslNettyClient != null) { + sslNettyClient.close(); + } + if (ambryRestServer != null) { + ambryRestServer.shutdown(); + } + } + + /** + * Clears any rows left from a prior run so the empty-prefix LIST assertions are deterministic across reruns. + */ + @Before + public void before() throws Exception { + Assume.assumeTrue(mysqlAvailable); + this.nettyClient = sslNettyClient; + Container container = ACCOUNT.getAllContainers().iterator().next(); + try (Connection connection = DriverManager.getConnection(NAMED_BLOB_DB_URL, NAMED_BLOB_DB_USER, + NAMED_BLOB_DB_PASSWORD); Statement statement = connection.createStatement()) { + statement.executeUpdate(String.format("DELETE FROM named_blobs_v2 WHERE account_id = %d AND container_id = %d", + ACCOUNT.getId(), container.getId())); + } + } + + /** + * PUT a few named blobs through the S3 stack, then issue an empty-prefix LIST (both v1 and v2 shapes) and assert + * 200 OK end-to-end. With {@code listNamedBlobsSqlOption=4}, an empty prefix collapses to a null prefix and runs the + * correlated-subquery LIST_ALL form against real MySQL. A regression in the S3-handler routing, the empty-prefix + * collapse, or the option-4 LIST_ALL SQL/param binding surfaces here as a non-200 (or a 500 on the window plan). + */ + @Test + public void s3EmptyPrefixListAgainstMySqlOption4Test() throws Exception { + String account = ACCOUNT.getName(); + Container container = ACCOUNT.getAllContainers().iterator().next(); + String containerName = container.getName(); + + String[] keys = new String[]{"empty_prefix_mysql_a", "empty_prefix_mysql_b", "empty_prefix_mysql_c"}; + int contentSize = 64; + for (String key : keys) { + doPutBlob(account, containerName, key, contentSize, TestUtils.getRandomBytes(contentSize)); + } + + // V1 LIST: GET /s3/{account}/{container}?prefix= (explicit empty value) + String uriV1 = String.format("/s3/%s/%s?prefix=", account, containerName); + FullHttpRequest reqV1 = buildRequest(HttpMethod.GET, uriV1, new DefaultHttpHeaders(), null); + HttpResponse respV1 = getHttpResponse(nettyClient.sendRequest(reqV1, null, null).get()); + assertEquals("Empty-prefix LIST (v1) through the S3 stack against MySQL option-4 LIST_ALL must return 200 OK", + HttpResponseStatus.OK, respV1.status()); + + // V2 LIST: GET /s3/{account}/{container}?prefix=&list-type=2 + String uriV2 = String.format("/s3/%s/%s?prefix=&list-type=2", account, containerName); + FullHttpRequest reqV2 = buildRequest(HttpMethod.GET, uriV2, new DefaultHttpHeaders(), null); + HttpResponse respV2 = getHttpResponse(nettyClient.sendRequest(reqV2, null, null).get()); + assertEquals("Empty-prefix LIST (v2) through the S3 stack against MySQL option-4 LIST_ALL must return 200 OK", + HttpResponseStatus.OK, respV2.status()); + } + + private void doPutBlob(String account, String container, String key, int contentSize, byte[] content) + throws Exception { + String uri = String.format("/s3/%s/%s/%s", account, container, key); + HttpHeaders headers = new DefaultHttpHeaders(); + headers.add(CONTENT_TYPE, OCTET_STREAM_CONTENT_TYPE); + headers.add(CONTENT_LENGTH, contentSize); + FullHttpRequest httpRequest = buildRequest(HttpMethod.PUT, uri, headers, ByteBuffer.wrap(content)); + HttpResponse response = getHttpResponse(nettyClient.sendRequest(httpRequest, null, null).get()); + assertEquals("Unexpected status putting seed blob " + key, HttpResponseStatus.OK, response.status()); + } +} diff --git a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java index a5970c593b..de6469ee87 100644 --- a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java +++ b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java @@ -30,6 +30,7 @@ import java.util.Set; import java.util.TimeZone; import java.util.concurrent.TimeUnit; +import org.junit.Assume; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -224,6 +225,187 @@ public void testListHidesBlobWhenLatestVersionIsExpired() throws Exception { + page.getEntries(), 0, page.getEntries().size()); } + /** + * Test case for list named blobs with a null prefix — the LIST_ALL_QUERY code path. + * + * Closes a coverage gap surfaced after #3265: prior to this test, every list() invocation in + * this integration suite passed a non-null prefix, so the no-prefix LIST code path + * (LIST_ALL_QUERY for options 2/3, LIST_ALL_SQL window-function variant for option 4) was + * never exercised by the int-test matrix. The empty-prefix LIST regression that motivated #3265 + * specifically crossed this path (S3 SDK sends `prefix=` empty → parseS3 collapses to null → list() with + * null prefix → LIST_ALL_QUERY under option 4), and option 4's window-function variant only + * shipped with unit-test coverage. This test runs against options 2/3/4 × hard-delete on/off + * via the existing parameterized matrix. + * + * Option-agnostic: PUT N distinct blobs in one container, LIST with null prefix, expect all N + * back in blob_name order. No multi-version / deleted-latest scenario here — see the + * follow-up testListAllNullPrefixHidesDeletedLatestUnderOption4 for that, which has + * different semantics across options 2/3 vs option 4 and so is option-4-only. + */ + @Test + public void testListNamedBlobsWithNullPrefix() throws Exception { + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + + // Seed N blobs with stable lexicographic names so we can assert ordering deterministically. + final int N = 5; + final String[] blobNames = new String[N]; + for (int i = 0; i < N; i++) { + blobNames[i] = String.format("testListAllNullPrefix-%02d-%s", i, TestUtils.getRandomKey(8)); + } + Arrays.sort(blobNames); + for (String name : blobNames) { + NamedBlobRecord record = new NamedBlobRecord(account.getName(), container.getName(), name, + getBlobId(account, container), + calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(record, NamedBlobState.READY, true).get(); + } + + // null prefix triggers LIST_ALL_QUERY (or LIST_ALL_SQL under option 4). This is the path + // an empty-prefix S3 LIST collapses to via parseS3. + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); + + assertEquals("Null-prefix LIST should return all " + N + " seeded blobs", + N, page.getEntries().size()); + for (int i = 0; i < N; i++) { + assertEquals("Null-prefix LIST should return blobs in blob_name ascending order", + blobNames[i], page.getEntries().get(i).getBlobName()); + } + } + + /** + * Option-4-only invariant test for the null-prefix path: if the latest READY version of a + * blob is TTL-expired (or soft-deleted), the blob must be hidden entirely from a null-prefix + * LIST. Under option 4, LIST_ALL_SQL applies the deleted_ts filter on the OUTER select after + * the window operator, so the second-latest non-deleted version is NOT surfaced — matching + * the LIST_WITH_PREFIX_SQL contract (already covered by testListHidesBlobWhenLatestVersionIsExpired). + * + * Options 2/3 LIST_ALL_QUERY has the opposite semantic (deleted_ts filter inside the inner + * subquery → second-latest surfaces). That divergence is intentional and is preserved by + * #3265 to avoid changing default behavior for fabrics still on options 2/3. So this test + * only runs under option 4. + */ + @Test + public void testListAllNullPrefixHidesDeletedLatestUnderOption4() throws Exception { + Assume.assumeTrue("Semantic only holds for option 4 LIST_ALL_SQL; options 2/3 surface " + + "the second-latest non-deleted version by design", listSqlOption == 4); + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + final String blobName = "testListAllNullPrefixHidesDeletedLatest"; + + // v1: older version, expires far in the future. + NamedBlobRecord v1 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v1, NamedBlobState.READY, true).get(); + + // Advance the mock clock so v2's generated version is strictly greater than v1's. + time.sleep(100); + + // v2: latest version, already expired at LIST time. + NamedBlobRecord v2 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() - TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v2, NamedBlobState.READY, true).get(); + + // null prefix → LIST_ALL_SQL under option 4. The blob must be hidden entirely. + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); + assertEquals("Option 4: latest version expired; null-prefix LIST must hide the blob entirely " + + "(no older-version leak). Got " + page.getEntries(), 0, page.getEntries().size()); + } + + /** + * Option-agnostic null-prefix pagination test. PUT N distinct blobs, then page through a null-prefix + * LIST with maxKeys < N and assert every page is in blob_name order with a correct continuation token, + * and that the concatenation across pages is exactly the N seeded blobs with no duplicates or gaps. + * + * This exercises the null-prefix continuation path on both the first page (pageToken == null → the + * {@code ? IS NULL} guard lists from the start) and subsequent pages (pageToken != null → + * {@code blob_name >= ?}). Under option 4 it covers the new correlated-subquery LIST_ALL binder + * (constructListAllQueryV4) on both code paths. + */ + @Test + public void testListAllNullPrefixPagination() throws Exception { + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + + final int N = 5; + final int pageSize = 2; + final String[] blobNames = new String[N]; + for (int i = 0; i < N; i++) { + blobNames[i] = String.format("testListAllNullPrefixPagination-%02d-%s", i, TestUtils.getRandomKey(8)); + } + Arrays.sort(blobNames); + for (String name : blobNames) { + NamedBlobRecord record = new NamedBlobRecord(account.getName(), container.getName(), name, + getBlobId(account, container), calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(record, NamedBlobState.READY, true).get(); + } + + List collected = new ArrayList<>(); + String pageToken = null; + int pageCount = 0; + do { + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, pageToken, pageSize).get(); + assertTrue("A non-final page must return exactly pageSize entries; final page returns the remainder", + page.getEntries().size() <= pageSize); + for (NamedBlobRecord record : page.getEntries()) { + collected.add(record.getBlobName()); + } + pageToken = page.getNextPageToken(); + assertTrue("Pagination did not terminate within the expected number of pages", ++pageCount <= N + 1); + } while (pageToken != null); + + assertEquals("Null-prefix pagination must return every seeded blob exactly once across pages", + Arrays.asList(blobNames), collected); + } + + /** + * Option-4-only positive counterpart to testListAllNullPrefixHidesDeletedLatestUnderOption4: when a blob + * has multiple non-deleted versions, a null-prefix LIST must return the LATEST version (not an older one). + * Confirms the correlated-subquery LIST_ALL form returns {@code candidate.version = MAX(version)}. + */ + @Test + public void testListAllNullPrefixReturnsLatestVersionUnderOption4() throws Exception { + Assume.assumeTrue("Multi-version null-prefix LIST_ALL exercised under option 4", listSqlOption == 4); + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + final String blobName = "testListAllNullPrefixReturnsLatest-" + TestUtils.getRandomKey(8); + + // v1: older version, far-future expiry (non-deleted). + NamedBlobRecord v1 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v1, NamedBlobState.READY, true).get(); + + // Advance the mock clock so v2's generated version is strictly greater than v1's. + time.sleep(100); + + // v2: latest version, far-future expiry (non-deleted). + NamedBlobRecord v2 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v2, NamedBlobState.READY, true).get(); + + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); + assertEquals("Null-prefix LIST must return exactly one row for a multi-version blob. Got " + + page.getEntries(), 1, page.getEntries().size()); + assertEquals("Null-prefix LIST must return the latest version's blob id (v2), not an older one", + v2.getBlobId(), page.getEntries().get(0).getBlobId()); + } + /** * Test case for list named blobs with prefix. * @throws Exception diff --git a/ambry-named-mysql/src/main/java/com/github/ambry/named/MySqlNamedBlobDb.java b/ambry-named-mysql/src/main/java/com/github/ambry/named/MySqlNamedBlobDb.java index 405500d038..fb3aa68520 100644 --- a/ambry-named-mysql/src/main/java/com/github/ambry/named/MySqlNamedBlobDb.java +++ b/ambry-named-mysql/src/main/java/com/github/ambry/named/MySqlNamedBlobDb.java @@ -318,11 +318,15 @@ private String getListWithPrefixSQLStatement(MySqlNamedBlobDbConfig config) { /** * Build the no-prefix LIST SQL. Selected by the same {@link MySqlNamedBlobDbConfig#listNamedBlobsSQLOption} * knob as {@link #getListWithPrefixSQLStatement}. Options 2 and 3 share the legacy INNER-JOIN + MAX-grouped - * subquery shape; option 4 uses a window-function shape that mirrors LIST_WITH_PREFIX_SQL option 4. + * subquery shape; option 4 uses a correlated-subquery shape that early-terminates at LIMIT (it mirrors + * LIST_WITH_PREFIX_SQL option 3's shape, minus the prefix predicate, while keeping option-4's + * hide-latest-deleted semantic). * * S3-handler change in linkedin/ambry#3260's follow-up normalizes empty-string prefix to null at the API - * layer, so any empty-prefix S3 LIST now arrives here. Option 4's LIST_WITH_PREFIX_SQL with - * {@code blob_name LIKE '%'} was the failure shape we are routing away from. + * layer, so any empty-prefix S3 LIST now arrives here. Two shapes were rejected for this no-prefix path: + * option 4's LIST_WITH_PREFIX_SQL with {@code blob_name LIKE '%'} (full-container scan), and the + * window-function variant ({@code MAX(version) OVER (PARTITION BY blob_name)}) which materializes the whole + * container's derived table before LIMIT and timed out on large containers (HTTP 500). */ private String getListAllSQLStatement(MySqlNamedBlobDbConfig config) { switch (config.listNamedBlobsSQLOption) { @@ -357,33 +361,39 @@ private String getListAllSQLStatement(MySqlNamedBlobDbConfig config) { // @formatter:on case 4: /** - * No-prefix LIST, window-function variant. Single PK range scan over (account_id, container_id) - * with MAX(version) OVER (PARTITION BY blob_name); no INNER JOIN, no GROUP BY materialization. + * No-prefix LIST, correlated-subquery variant (mirrors LIST_WITH_PREFIX_SQL option 3's shape, + * minus the prefix predicate). The outer scans candidate rows in PRIMARY KEY (blob_name) order and + * keeps only the row whose version equals the per-name MAX(version). The deleted_ts predicate sits + * on the OUTER candidate, so a soft-deleted latest version hides the blob entirely — no older + * non-deleted version can substitute (its version != MAX) — matching LIST_WITH_PREFIX_SQL option 4 + * semantics. Operators flipping from option 3 to option 4 inherit the consistent hide-latest-deleted + * semantic for the no-prefix path too. * - * Correctness invariant — matches LIST_WITH_PREFIX_SQL option 4: deleted_ts predicate is applied - * on the OUTER select after the window operator, so a soft-deleted latest version causes the - * blob to be hidden entirely (rather than surfacing an older non-deleted version). This unifies - * the no-prefix and with-prefix LIST semantics under option 4. Operators flipping from option 3 - * to option 4 inherit both the perf improvement and the consistent hide-latest-deleted semantic. - * - * Requires MySQL 8.0+ or TiDB (window function). + * Unlike the window-function shape, this plan does NOT materialize the whole container. With the PK + * already ordered by blob_name, "ORDER BY blob_name LIMIT N" early-terminates after N matches instead + * of computing MAX(version) OVER the entire partition. That is what keeps an empty-prefix LIST on a + * large container bounded (first-page cost ~ O(N + skipped deleted-latest names)) rather than + * O(container) — the prior window variant materialized the full derived table before LIMIT and timed + * out on large containers (Communications link failure -> HTTP 500). */ // @formatter:off return String.format("" - + "SELECT blob_name, blob_id, version, deleted_ts, blob_size, modified_ts " - + "FROM ( " - + " SELECT blob_name, blob_id, version, deleted_ts, blob_size, modified_ts, " - + " MAX(version) OVER (PARTITION BY blob_name) AS max_version " - + " FROM named_blobs_v2 " - + " WHERE account_id = ? " // 1 - + " AND container_id = ? " // 2 - + " AND %1$s " // blob_state = x - + " AND ( ? IS NULL OR blob_name >= ? ) " // 3, 4 (pageToken twice) - + ") t " - + "WHERE version = max_version " - + " AND (deleted_ts IS NULL OR deleted_ts > %2$s) " - + "ORDER BY blob_name " - + "LIMIT ?", STATE_MATCH, CURRENT_TIME); // 5 + + "SELECT candidate.blob_name, candidate.blob_id, candidate.version, candidate.deleted_ts, candidate.blob_size, candidate.modified_ts " + + "FROM named_blobs_v2 candidate " + + "WHERE candidate.account_id = ? " // 1 + + " AND candidate.container_id = ? " // 2 + + " AND candidate.%1$s " // blob_state = x + + " AND ( ? IS NULL OR candidate.blob_name >= ? ) " // 3, 4 (pageToken twice) + + " AND (candidate.deleted_ts IS NULL OR candidate.deleted_ts > %2$s) " + + " AND candidate.version = ( " + + " SELECT MAX(latest.version) " + + " FROM named_blobs_v2 latest " + + " WHERE latest.account_id = ? " // 5 + + " AND latest.container_id = ? " // 6 + + " AND latest.blob_name = candidate.blob_name " + + " AND latest.%1$s ) " + + "ORDER BY candidate.blob_name " + + "LIMIT ?", STATE_MATCH, CURRENT_TIME); // 7 // @formatter:on default: throw new IllegalArgumentException("Invalid listNamedBlobsSQLOption: " + config.listNamedBlobsSQLOption); @@ -866,7 +876,20 @@ private Page run_list_v2(String accountName, String containerNa int maxKeysValue = maxKeys == null ? config.listMaxResults : maxKeys; try (PreparedStatement statement = connection.prepareStatement(queryStatement)) { if (blobNamePrefix == null) { - constructListAllQuery(statement, accountId, containerId, pageToken, maxKeysValue); + // The no-prefix LIST_ALL_SQL placeholder count differs by option: options 2/3 bind 5 params, + // option 4 (correlated subquery) binds 7. Keep the binder in lockstep with getListAllSQLStatement. + switch (config.listNamedBlobsSQLOption) { + case 2: + case 3: + constructListAllQuery(statement, accountId, containerId, pageToken, maxKeysValue); + break; + case 4: + constructListAllQueryV4(statement, accountId, containerId, pageToken, maxKeysValue); + break; + default: + throw new IllegalStateException( + "Invalid listNamedBlobsSQLOption: " + config.listNamedBlobsSQLOption); + } } else { switch (config.listNamedBlobsSQLOption) { case 2: @@ -883,6 +906,12 @@ private Page run_list_v2(String accountName, String containerNa "Invalid listNamedBlobsSQLOption: " + config.listNamedBlobsSQLOption); } } + // Bound the LIST against an unexpectedly large container. With a positive timeout the driver cancels the + // statement and the server throws a SQLException (cleanly), instead of the query running past a shorter + // network/socket timeout and tearing the connection ("Communications link failure" -> HTTP 500). + if (config.listQueryTimeoutSeconds > 0) { + statement.setQueryTimeout(config.listQueryTimeoutSeconds); + } query = statement.toString(); logger.debug("Getting list of blobs matching prefix {} from MySql. Query {}", blobNamePrefix, query); metricsRecoder.namedBlobListRate.mark(); @@ -924,7 +953,7 @@ private Page run_list_v2(String accountName, String containerNa */ private void constructListAllQuery(PreparedStatement statement, short accountId, short containerId, String pageToken, int maxKeysValue) throws SQLException { - // list-all no prefix + // list-all no prefix (options 2/3 legacy INNER JOIN form, 5 params) statement.setInt(1, accountId); statement.setInt(2, containerId); statement.setString(3, pageToken); @@ -932,6 +961,30 @@ private void constructListAllQuery(PreparedStatement statement, short accountId, statement.setInt(5, maxKeysValue + 1); } + /** + * Construct the no-prefix LIST_ALL query when {@link MySqlNamedBlobDbConfig#listNamedBlobsSQLOption} is 4. + * Option 4's no-prefix form is a correlated subquery (see {@link #getListAllSQLStatement}) and binds seven + * parameters: (account_id, container_id, pageToken for the NULL guard, pageToken for blob_name >= cursor, + * subquery account_id, subquery container_id, LIMIT). A null pageToken (first page) short-circuits the + * {@code ? IS NULL} guard so the scan lists from the start of the container. + * @param statement The {@link PreparedStatement} to set the parameters on. + * @param accountId The account id + * @param containerId The container id + * @param pageToken The page token (null on the first page) + * @param maxKeysValue The max key to return + * @throws SQLException + */ + private void constructListAllQueryV4(PreparedStatement statement, short accountId, short containerId, + String pageToken, int maxKeysValue) throws SQLException { + statement.setInt(1, accountId); + statement.setInt(2, containerId); + statement.setString(3, pageToken); + statement.setString(4, pageToken); + statement.setInt(5, accountId); + statement.setInt(6, containerId); + statement.setInt(7, maxKeysValue + 1); + } + /** * Construct a list query statement with prefix when {@link MySqlNamedBlobDbConfig#listNamedBlobsSQLOption} is 2 * @param statement The {@link PreparedStatement} to set the parameters on.