Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid infinite number of calls to listBlobs when doing prefix removals (e..g, gridset or layer removals) #1232

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Avoid infinite number of calls to listBlobs when doing prefix removals (e..g, gridset or layer removals) #1232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions geowebcache/azureblob/src/main/java/org/geowebcache/azure/AzureBlobStore.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scheduled deletes stored in property files would not get removed if the folder was empty.

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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix for the high number of listBlob. It would just not get out of this loop.

// 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
Open in desktop
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(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are changes in Mockito behavior, previous version was matching null too, the current one does not.

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
Open in desktop
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
Loading