Skip to content

Commit

Permalink
Avoid infinite number of calls to listBlobs when doing prefix removal…
Browse files Browse the repository at this point in the history 
…s (e..g, gridset or layer removals)
  • Loading branch information
@aaime @github-actions
aaime authored and github-actions[bot] committed Apr 19, 2024
1 parent 079a2f2 commit 47a421f
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 28 deletions.
4 changes: 2 additions & 2 deletions geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand Down
19 changes: 11 additions & 8 deletions geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
22 changes: 15 additions & 7 deletions geowebcache/azureblob/src/main/java/org/geowebcache/azure/DeleteManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -169,14 +172,21 @@ public void issuePendingBulkDeletes() throws StorageException {

try {
Properties deletes = client.getProperties(pendingDeletesKey);
Set<String> deletesToClear = new HashSet<>();
for (Map.Entry<Object, Object> e : deletes.entrySet()) {
final String prefix = e.getKey().toString();
final long timestamp = Long.parseLong(e.getValue().toString());
log.info(
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 {
Expand Down Expand Up @@ -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(
Expand Down
30 changes: 20 additions & 10 deletions .../azureblob/src/test/java/org/geowebcache/azure/AbstractAzureBlobStoreIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -397,7 +400,7 @@ public void testTruncateShortCutsIfNoTilesInParametersPrefix()
anyString(),
anyString(),
anyString(),
anyString(),
anyStringOrNull(),
anyLong(),
anyLong(),
anyInt(),
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)));
Expand Down
4 changes: 3 additions & 1 deletion geowebcache/core/src/main/java/org/geowebcache/util/TMSKeyBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 47a421f

Please sign in to comment.