From af687a74802cadef1c16294662896032e002f00b Mon Sep 17 00:00:00 2001 From: Sophie Guo Date: Tue, 7 May 2024 12:26:27 -0700 Subject: [PATCH 1/2] Support maxKeys for list request --- .../ambry/config/MySqlNamedBlobDbConfig.java | 2 +- .../com/github/ambry/named/NamedBlobDb.java | 17 ++++++++++------- .../ambry/frontend/NamedBlobListHandler.java | 5 ++++- .../FrontendRestRequestServiceTest.java | 7 +++---- .../github/ambry/frontend/TestNamedBlobDb.java | 5 +++-- .../named/MySqlNamedBlobDbIntegrationTest.java | 17 ++++++++++++----- .../github/ambry/named/MySqlNamedBlobDb.java | 14 ++++++++------ .../tools/perf/NamedBlobMysqlDatabasePerf.java | 4 ++-- 8 files changed, 43 insertions(+), 28 deletions(-) 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 b15e301261..692addec1d 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 @@ -55,7 +55,7 @@ public class MySqlNamedBlobDbConfig { * The maximum number of entries to return per response page when listing blobs. */ @Config(LIST_MAX_RESULTS) - @Default("100") + @Default("1000") public final int listMaxResults; /** diff --git a/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java b/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java index a309741e1d..d7849e61aa 100644 --- a/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java +++ b/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java @@ -53,17 +53,20 @@ default CompletableFuture get(String accountName, String contai /** * List blobs that start with a provided prefix in a container. This returns paginated results. If there are * additional pages to read, {@link Page#getNextPageToken()} will be non null. - * @param accountName the name of the account. - * @param containerName the name of the container. + * + * @param accountName the name of the account. + * @param containerName the name of the container. * @param blobNamePrefix the name prefix to search for. - * @param pageToken if {@code null}, return the first page of {@link NamedBlobRecord}s that start with - * {@code blobNamePrefix}. If set, use this as a token to resume reading additional pages - * of records that start with the prefix. + * @param pageToken if {@code null}, return the first page of {@link NamedBlobRecord}s that start with + * {@code blobNamePrefix}. If set, use this as a token to resume reading additional pages of + * records that start with the prefix. + * @param maxKey the maximum number of keys returned in the response. By default, the action returns up to listMaxResults + * which can be tuned by config. * @return a {@link CompletableFuture} that will eventually contain a {@link Page} of {@link NamedBlobRecord}s - * starting with the specified prefix or an exception if an error occurred. + * starting with the specified prefix or an exception if an error occurred. */ CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken); + String pageToken, String maxKey); /** * Persist a {@link NamedBlobRecord} in the database. diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java index f355443926..d1958e7481 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java @@ -31,6 +31,8 @@ import org.slf4j.LoggerFactory; import static com.github.ambry.frontend.FrontendUtils.*; +import static com.github.ambry.frontend.s3.S3ListHandler.*; +import static com.github.ambry.rest.RestUtils.*; /** @@ -129,9 +131,10 @@ private Callback securityProcessRequestCallback() { private Callback securityPostProcessRequestCallback() { return buildCallback(frontendMetrics.listSecurityPostProcessRequestMetrics, securityCheckResult -> { NamedBlobPath namedBlobPath = NamedBlobPath.parse(RestUtils.getRequestPath(restRequest), restRequest.getArgs()); + String maxKeys = getHeader(restRequest.getArgs(), MAXKEYS_PARAM_NAME, false); CallbackUtils.callCallbackAfter( namedBlobDb.list(namedBlobPath.getAccountName(), namedBlobPath.getContainerName(), - namedBlobPath.getBlobNamePrefix(), namedBlobPath.getPageToken()), listBlobsCallback()); + namedBlobPath.getBlobNamePrefix(), namedBlobPath.getPageToken(), maxKeys), listBlobsCallback()); }, uri, LOGGER, finalCallback); } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index fd90dea15e..012c4d5a42 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -41,7 +41,6 @@ import com.github.ambry.config.QuotaConfig; import com.github.ambry.config.RouterConfig; import com.github.ambry.config.VerifiableProperties; -import com.github.ambry.frontend.s3.S3MultipartUploadHandler; import com.github.ambry.messageformat.BlobInfo; import com.github.ambry.messageformat.BlobProperties; import com.github.ambry.named.DeleteResult; @@ -2525,17 +2524,17 @@ private void doListNamedBlobsTest(String prefix, String pageToken, Page> future = new CompletableFuture<>(); future.completeExceptionally(new RestServiceException("NamedBlobDb error", expectedErrorCode)); - when(namedBlobDb.list(any(), any(), any(), any())).thenReturn(future); + when(namedBlobDb.list(any(), any(), any(), any(), any())).thenReturn(future); } if (expectedErrorCode == null) { assertNotNull("pageToReturn should be set", pageToReturn); doOperation(restRequest, restResponseChannel); - verify(namedBlobDb).list(refAccount.getName(), refContainer.getName(), prefix, pageToken); + verify(namedBlobDb).list(refAccount.getName(), refContainer.getName(), prefix, pageToken, null); Page response = Page.fromJson(new JSONObject(new String(restResponseChannel.getResponseBody())), NamedBlobListEntry::new); assertEquals("Unexpected blobs returned", diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java index f70eacd34f..780950b169 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java @@ -83,7 +83,7 @@ public CompletableFuture get(String accountName, String contain @Override public CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken) { + String pageToken, String maxKey) { if (exception != null) { return FutureUtils.completedExceptionally(exception); } @@ -113,7 +113,8 @@ public CompletableFuture> list(String accountName, String NamedBlobRecord record = recordList.get(recordList.size() - 1).getFirst(); long deleteTs = recordList.get(recordList.size() - 1).getSecond().getSecond(); - if (numRecords++ == listMaxResults) { + int maxKeysValue = maxKey == null ? listMaxResults : Integer.parseInt(maxKey); + if (numRecords++ == maxKeysValue) { nextContinuationToken = record.getBlobName(); break; } diff --git a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java index 4bed94197c..1f7a2ff14e 100644 --- a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java +++ b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java @@ -122,7 +122,7 @@ public void testPutGetListDeleteSequence() throws Exception { time.setCurrentMilliseconds(System.currentTimeMillis()); for (Account account : accountService.getAllAccounts()) { for (Container container : account.getAllContainers()) { - Page page = namedBlobDb.list(account.getName(), container.getName(), "name", null).get(); + Page page = namedBlobDb.list(account.getName(), container.getName(), "name", null, null).get(); assertNull("No continuation token expected", page.getNextPageToken()); assertEquals("Unexpected number of blobs in container", blobsPerContainer, page.getEntries().size()); } @@ -133,12 +133,19 @@ public void testPutGetListDeleteSequence() throws Exception { for (Account account : accountService.getAllAccounts()) { for (Container container : account.getAllContainers()) { //page with no token - Page page = namedBlobDb.list(account.getName(), container.getName(), null, null).get(); + Page page = namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); assertNull("No continuation token expected", page.getNextPageToken()); assertEquals("Unexpected number of blobs in container", blobsPerContainer, page.getEntries().size()); //page with token - Page pageWithToken = namedBlobDb.list(account.getName(), container.getName(), null, "name/4").get(); - assertEquals("Unexpected number of blobs in container", blobsPerContainer / 5, pageWithToken.getEntries().size()); + Page pageWithToken = + namedBlobDb.list(account.getName(), container.getName(), null, "name/4", null).get(); + assertEquals("Unexpected number of blobs in container", blobsPerContainer / 5, + pageWithToken.getEntries().size()); + //page with maxKeys + Page pageWithMaxKey = + namedBlobDb.list(account.getName(), container.getName(), null, null, "1").get(); + assertEquals("Unexpected number of blobs in container", blobsPerContainer / 5, + pageWithMaxKey.getEntries().size()); } } @@ -293,7 +300,7 @@ public void testListNamedBlobs() throws Exception { ); // List named blob should only put out valid ones without empty entries. - Page page = namedBlobDb.list(account.getName(), container.getName(), blobNamePrefix, null).get(); + Page page = namedBlobDb.list(account.getName(), container.getName(), blobNamePrefix, null, null).get(); assertEquals("List named blob entries should match the valid records", validRecords, new HashSet<>(page.getEntries())); assertNull("Next page token should be null", page.getNextPageToken()); } 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 5fd22a3216..468f34a978 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 @@ -342,11 +342,12 @@ public CompletableFuture get(String accountName, String contain @Override public CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken) { + String pageToken, String maxKeys) { return executeTransactionAsync(accountName, containerName, true, (accountId, containerId, connection) -> { long startTime = this.time.milliseconds(); Page recordPage = - run_list_v2(accountName, containerName, blobNamePrefix, pageToken, accountId, containerId, connection); + run_list_v2(accountName, containerName, blobNamePrefix, pageToken, accountId, containerId, connection, + maxKeys); metricsRecoder.namedBlobListTimeInMs.update(this.time.milliseconds() - startTime); return recordPage; }, null); @@ -623,9 +624,10 @@ private NamedBlobRecord run_get_v2(String accountName, String containerName, Str } private Page run_list_v2(String accountName, String containerName, String blobNamePrefix, - String pageToken, short accountId, short containerId, Connection connection) throws Exception { + String pageToken, short accountId, short containerId, Connection connection, String maxKeys) throws Exception { String query = ""; - String queryStatement = blobNamePrefix == null? LIST_ALL_QUERY_V2: LIST_QUERY_V2; + String queryStatement = blobNamePrefix == null ? LIST_ALL_QUERY_V2 : LIST_QUERY_V2; + int maxKeysValue = maxKeys == null ? config.listMaxResults : Integer.parseInt(maxKeys); try (PreparedStatement statement = connection.prepareStatement(queryStatement)) { statement.setInt(1, accountId); statement.setInt(2, containerId); @@ -636,7 +638,7 @@ private Page run_list_v2(String accountName, String containerNa statement.setString(3, blobNamePrefix + "%"); statement.setString(4, pageToken != null ? pageToken : blobNamePrefix); } - statement.setInt(5, config.listMaxResults + 1); + statement.setInt(5, maxKeysValue + 1); query = statement.toString(); logger.debug("Getting list of blobs matching prefix {} from MySql. Query {}", blobNamePrefix, query); try (ResultSet resultSet = statement.executeQuery()) { @@ -645,7 +647,7 @@ private Page run_list_v2(String accountName, String containerNa int resultIndex = 0; while (resultSet.next()) { String blobName = resultSet.getString(1); - if (resultIndex++ == config.listMaxResults) { + if (resultIndex++ == maxKeysValue) { nextContinuationToken = blobName; break; } diff --git a/ambry-tools/src/main/java/com/github/ambry/tools/perf/NamedBlobMysqlDatabasePerf.java b/ambry-tools/src/main/java/com/github/ambry/tools/perf/NamedBlobMysqlDatabasePerf.java index d7bc97bfef..274584d5e6 100644 --- a/ambry-tools/src/main/java/com/github/ambry/tools/perf/NamedBlobMysqlDatabasePerf.java +++ b/ambry-tools/src/main/java/com/github/ambry/tools/perf/NamedBlobMysqlDatabasePerf.java @@ -529,7 +529,7 @@ public void run() { int numberOfList = 0; for (NamedBlobRecord record : allRecords) { if (!record.getAccountName().equals(String.format(ACCOUNT_NAME_FORMAT, HUGE_LIST_ACCOUNT_ID))) { - namedBlobDb.list(record.getAccountName(), record.getContainerName(), "A", null).get(); + namedBlobDb.list(record.getAccountName(), record.getContainerName(), "A", null, null).get(); numberOfList++; if (numberOfList == 100) { break; @@ -543,7 +543,7 @@ public void run() { String token = null; for (int i = 0; i < 100; i++) { token = - namedBlobDb.list(accountName, containerName, HUGE_LIST_COMMON_PREFIX, token).get().getNextPageToken(); + namedBlobDb.list(accountName, containerName, HUGE_LIST_COMMON_PREFIX, token, null).get().getNextPageToken(); } System.out.println("PerformanceTestWorker " + id + " finishes listing for huge records"); } From 1653d3aa66375f68a0c73562604f04f8c019db73 Mon Sep 17 00:00:00 2001 From: Sophie Guo Date: Thu, 9 May 2024 15:31:20 -0700 Subject: [PATCH 2/2] Address comments --- .../src/main/java/com/github/ambry/named/NamedBlobDb.java | 2 +- .../com/github/ambry/frontend/NamedBlobListHandler.java | 3 ++- .../java/com/github/ambry/frontend/TestNamedBlobDb.java | 4 ++-- .../github/ambry/named/MySqlNamedBlobDbIntegrationTest.java | 2 +- .../main/java/com/github/ambry/named/MySqlNamedBlobDb.java | 6 +++--- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java b/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java index d7849e61aa..5c94ef076a 100644 --- a/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java +++ b/ambry-api/src/main/java/com/github/ambry/named/NamedBlobDb.java @@ -66,7 +66,7 @@ default CompletableFuture get(String accountName, String contai * starting with the specified prefix or an exception if an error occurred. */ CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken, String maxKey); + String pageToken, Integer maxKey); /** * Persist a {@link NamedBlobRecord} in the database. diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java index d1958e7481..c10e3bb20c 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobListHandler.java @@ -134,7 +134,8 @@ private Callback securityPostProcessRequestCallback() { String maxKeys = getHeader(restRequest.getArgs(), MAXKEYS_PARAM_NAME, false); CallbackUtils.callCallbackAfter( namedBlobDb.list(namedBlobPath.getAccountName(), namedBlobPath.getContainerName(), - namedBlobPath.getBlobNamePrefix(), namedBlobPath.getPageToken(), maxKeys), listBlobsCallback()); + namedBlobPath.getBlobNamePrefix(), namedBlobPath.getPageToken(), + maxKeys == null ? null : Integer.parseInt(maxKeys)), listBlobsCallback()); }, uri, LOGGER, finalCallback); } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java index 780950b169..4515868992 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/TestNamedBlobDb.java @@ -83,7 +83,7 @@ public CompletableFuture get(String accountName, String contain @Override public CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken, String maxKey) { + String pageToken, Integer maxKey) { if (exception != null) { return FutureUtils.completedExceptionally(exception); } @@ -113,7 +113,7 @@ public CompletableFuture> list(String accountName, String NamedBlobRecord record = recordList.get(recordList.size() - 1).getFirst(); long deleteTs = recordList.get(recordList.size() - 1).getSecond().getSecond(); - int maxKeysValue = maxKey == null ? listMaxResults : Integer.parseInt(maxKey); + int maxKeysValue = maxKey == null ? listMaxResults : maxKey; if (numRecords++ == maxKeysValue) { nextContinuationToken = record.getBlobName(); break; diff --git a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java index 1f7a2ff14e..f727633988 100644 --- a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java +++ b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbIntegrationTest.java @@ -143,7 +143,7 @@ public void testPutGetListDeleteSequence() throws Exception { pageWithToken.getEntries().size()); //page with maxKeys Page pageWithMaxKey = - namedBlobDb.list(account.getName(), container.getName(), null, null, "1").get(); + namedBlobDb.list(account.getName(), container.getName(), null, null, 1).get(); assertEquals("Unexpected number of blobs in container", blobsPerContainer / 5, pageWithMaxKey.getEntries().size()); } 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 468f34a978..2751d13f60 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 @@ -342,7 +342,7 @@ public CompletableFuture get(String accountName, String contain @Override public CompletableFuture> list(String accountName, String containerName, String blobNamePrefix, - String pageToken, String maxKeys) { + String pageToken, Integer maxKeys) { return executeTransactionAsync(accountName, containerName, true, (accountId, containerId, connection) -> { long startTime = this.time.milliseconds(); Page recordPage = @@ -624,10 +624,10 @@ private NamedBlobRecord run_get_v2(String accountName, String containerName, Str } private Page run_list_v2(String accountName, String containerName, String blobNamePrefix, - String pageToken, short accountId, short containerId, Connection connection, String maxKeys) throws Exception { + String pageToken, short accountId, short containerId, Connection connection, Integer maxKeys) throws Exception { String query = ""; String queryStatement = blobNamePrefix == null ? LIST_ALL_QUERY_V2 : LIST_QUERY_V2; - int maxKeysValue = maxKeys == null ? config.listMaxResults : Integer.parseInt(maxKeys); + int maxKeysValue = maxKeys == null ? config.listMaxResults : maxKeys; try (PreparedStatement statement = connection.prepareStatement(queryStatement)) { statement.setInt(1, accountId); statement.setInt(2, containerId);