From 47a421f157ccd0568a65660b995a13361a1fcc31 Mon Sep 17 00:00:00 2001 From: Andrea Aime Date: Thu, 18 Apr 2024 13:03:36 +0200 Subject: [PATCH] Avoid infinite number of calls to listBlobs when doing prefix removals (e..g, gridset or layer removals) --- .../org/geowebcache/azure/AzureBlobStore.java | 4 +-- .../org/geowebcache/azure/AzureClient.java | 19 +++++++----- .../org/geowebcache/azure/DeleteManager.java | 22 +++++++++----- ...AbstractAzureBlobStoreIntegrationTest.java | 30 ++++++++++++------- .../org/geowebcache/util/TMSKeyBuilder.java | 4 ++- 5 files changed, 51 insertions(+), 28 deletions(-) diff --git a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java index 096be5d7e..3a9eada75 100644 --- a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java +++ b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java @@ -67,7 +67,7 @@ public class AzureBlobStore implements BlobStore { private final TMSKeyBuilder keyBuilder; private final BlobStoreListenerList listeners = new BlobStoreListenerList(); private final AzureClient client; - private DeleteManager deleteManager; + DeleteManager deleteManager; private volatile boolean shutDown = false; @@ -200,7 +200,7 @@ public boolean delete(TileObject obj) throws StorageException { @Override public boolean delete(TileRange tileRange) throws StorageException { // see if there is anything to delete in that range by computing a prefix - final String coordsPrefix = keyBuilder.coordinatesPrefix(tileRange, true); + final String coordsPrefix = keyBuilder.coordinatesPrefix(tileRange, false); if (client.listBlobs(coordsPrefix, 1).isEmpty()) { return false; } diff --git a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureClient.java b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureClient.java index 4a5ac008d..2c5bae344 100644 --- a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureClient.java +++ b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureClient.java @@ -94,14 +94,17 @@ public AzureClient(AzureBlobStoreData configuration) throws StorageException { // no way to see if the containerURL already exists, try to create and see if // we get a 409 CONFLICT try { - int status = this.container.create(null, null, null).blockingGet().statusCode(); - if (!HttpStatus.valueOf(status).is2xxSuccessful() - && status != HttpStatus.CONFLICT.value()) { - throw new StorageException( - "Failed to create container " - + containerName - + ", REST API returned a " - + status); + int status = this.container.getProperties().blockingGet().statusCode(); + if (status == HttpStatus.NOT_FOUND.value()) { + status = this.container.create(null, null, null).blockingGet().statusCode(); + if (!HttpStatus.valueOf(status).is2xxSuccessful() + && status != HttpStatus.CONFLICT.value()) { + throw new StorageException( + "Failed to create container " + + containerName + + ", REST API returned a " + + status); + } } } catch (RestException e) { if (e.response().statusCode() != HttpStatus.CONFLICT.value()) { diff --git a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/DeleteManager.java b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/DeleteManager.java index 2c8cd2213..aaa1e585e 100644 --- a/geowebcache/azureblob/src/main/java/org/geowebcache/azure/DeleteManager.java +++ b/geowebcache/azureblob/src/main/java/org/geowebcache/azure/DeleteManager.java @@ -17,6 +17,7 @@ import static org.geowebcache.azure.AzureBlobStore.log; import static org.springframework.http.HttpStatus.NOT_FOUND; +import com.google.common.base.Strings; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.microsoft.azure.storage.blob.ContainerURL; import com.microsoft.azure.storage.blob.ListBlobsOptions; @@ -26,9 +27,11 @@ import com.microsoft.rest.v2.RestException; import java.io.Closeable; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; @@ -169,6 +172,7 @@ public void issuePendingBulkDeletes() throws StorageException { try { Properties deletes = client.getProperties(pendingDeletesKey); + Set deletesToClear = new HashSet<>(); for (Map.Entry e : deletes.entrySet()) { final String prefix = e.getKey().toString(); final long timestamp = Long.parseLong(e.getValue().toString()); @@ -176,7 +180,13 @@ public void issuePendingBulkDeletes() throws StorageException { String.format( "Restarting pending bulk delete on '%s/%s':%d", client.getContainerName(), prefix, timestamp)); - asyncDelete(prefix, timestamp); + if (!asyncDelete(prefix, timestamp)) { + deletesToClear.add(prefix); + } + } + if (!deletesToClear.isEmpty()) { + deletes.keySet().removeAll(deletesToClear); + client.putProperties(pendingDeletesKey, deletes); } } finally { try { @@ -281,12 +291,10 @@ public Long call() throws Exception { checkInterrupted(); deleteItems(container, response.body().segment(), filter); String marker = response.body().nextMarker(); - if (marker != null) { - response = - container.listBlobsFlatSegment(marker, options, null).blockingGet(); - } else { - break; - } + // marker will be empty if there is no next page + if (Strings.isNullOrEmpty(marker)) break; + // fetch next page + response = container.listBlobsFlatSegment(marker, options, null).blockingGet(); } } catch (InterruptedException | IllegalStateException e) { log.info( diff --git a/geowebcache/azureblob/src/test/java/org/geowebcache/azure/AbstractAzureBlobStoreIntegrationTest.java b/geowebcache/azureblob/src/test/java/org/geowebcache/azure/AbstractAzureBlobStoreIntegrationTest.java index a06df3bcf..9ddb3aded 100644 --- a/geowebcache/azureblob/src/test/java/org/geowebcache/azure/AbstractAzureBlobStoreIntegrationTest.java +++ b/geowebcache/azureblob/src/test/java/org/geowebcache/azure/AbstractAzureBlobStoreIntegrationTest.java @@ -19,10 +19,12 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.mockito.AdditionalMatchers.or; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -36,6 +38,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.logging.Logger; import org.geotools.util.logging.Logging; @@ -154,7 +157,7 @@ public void testPutWithListener() throws MimeException, StorageException { eq(tile.getLayerName()), eq(tile.getGridSetId()), eq(tile.getBlobFormat()), - anyString(), + anyStringOrNull(), eq(20L), eq(30L), eq(12), @@ -171,7 +174,7 @@ public void testPutWithListener() throws MimeException, StorageException { eq(tile.getLayerName()), eq(tile.getGridSetId()), eq(tile.getBlobFormat()), - anyString(), + anyStringOrNull(), eq(20L), eq(30L), eq(12), @@ -212,7 +215,7 @@ public void testDelete() throws MimeException, StorageException { eq(tile.getLayerName()), eq(tile.getGridSetId()), eq(tile.getBlobFormat()), - anyString(), + anyStringOrNull(), eq(22L), eq(30L), eq(12), @@ -397,7 +400,7 @@ public void testTruncateShortCutsIfNoTilesInParametersPrefix() anyString(), anyString(), anyString(), - anyString(), + anyStringOrNull(), anyLong(), anyLong(), anyInt(), @@ -493,21 +496,26 @@ public void testTruncateRespectsLevels() throws StorageException, MimeException verify(listener, times(expectedCount)) .tileDeleted( - anyString(), - anyString(), - anyString(), - anyString(), + anyStringOrNull(), + anyStringOrNull(), + anyStringOrNull(), + anyStringOrNull(), anyLong(), anyLong(), anyInt(), anyLong()); } + private static String anyStringOrNull() { + return or(isNull(), anyString()); + } + /** * If there are not {@link BlobStoreListener}s, use an optimized code path (not calling delete() * for each tile) */ @Test + @SuppressWarnings("unchecked") public void testTruncateOptimizationIfNoListeners() throws StorageException, MimeException { final int zoomStart = 0; @@ -537,10 +545,12 @@ public void testTruncateOptimizationIfNoListeners() throws StorageException, Mim mimeType, parameters); - blobStore = Mockito.spy(blobStore); + @SuppressWarnings("PMD.CloseResource") // closed by the store + DeleteManager deleteManager = Mockito.spy(blobStore.deleteManager); assertTrue(blobStore.delete(tileRange)); - verify(blobStore, times(0)).delete(Mockito.any(TileObject.class)); + verify(deleteManager, times(0)).executeParallel(Mockito.any(List.class)); + assertFalse(blobStore.get(queryTile(0, 0, 0))); assertFalse(blobStore.get(queryTile(0, 0, 1))); assertFalse(blobStore.get(queryTile(0, 1, 1))); diff --git a/geowebcache/core/src/main/java/org/geowebcache/util/TMSKeyBuilder.java b/geowebcache/core/src/main/java/org/geowebcache/util/TMSKeyBuilder.java index e8ac9b1eb..c7b1f81f9 100644 --- a/geowebcache/core/src/main/java/org/geowebcache/util/TMSKeyBuilder.java +++ b/geowebcache/core/src/main/java/org/geowebcache/util/TMSKeyBuilder.java @@ -39,6 +39,7 @@ public final class TMSKeyBuilder { public static final String LAYER_METADATA_OBJECT_NAME = "metadata.properties"; public static final String PARAMETERS_METADATA_OBJECT_PREFIX = "parameters-"; public static final String PARAMETERS_METADATA_OBJECT_SUFFIX = ".properties"; + public static final String PENDING_DELETES = "_pending_deletes.properties"; private String prefix; @@ -220,7 +221,8 @@ public String coordinatesPrefix(TileRange obj, boolean endWithSlash) { } public String pendingDeletes() { - return String.format("%s/%s", prefix, "_pending_deletes.properties"); + if (!Strings.isNullOrEmpty(prefix)) return String.format("%s/%s", prefix, PENDING_DELETES); + else return PENDING_DELETES; } private static String join(boolean closing, Object... elements) {