Limit number of S3 requests when truncating based on a tile range #1390
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…returned when deleting tile cash. Use list-objects and delete-objects batch method to operate on 1000 sized batches where possible. Ensure that tile caches are informed of changes through listeners Fix Integration tests that exercise delete file paths
…returned when deleting tile cash. Use list-objects and delete-objects batch method to operate on 1000 sized batches where possible. Ensure that tile caches are informed of changes through listeners Fix Integration tests that exercise delete file paths
…returned when deleting tile cash. Use list-objects and delete-objects batch method to operate on 1000 sized batches where possible. Ensure that tile caches are informed of changes through listeners Fix Integration tests that exercise delete file paths
aaime
requested changes
Apr 14, 2025
Member
There was a problem hiding this comment.
Checked. Much easier to understand than the previous one, thanks a lot.
Three main issues:
- The code seems to be ignoring the TileRange bounding box, if set (it should not ignore it if it's a subset of the gridset one, e.g., if the user asked to remove a small portion of the data)
- Some notifications will be issued "twice" along two different avenues, this may cause disk quota to go off synch
- I don't see a test checking that the requests are no longer originating 404s
|
|
||
| @Override | ||
| public void put(TileObject obj) throws StorageException { | ||
| TMSKeyBuilder.buildParametersId(obj); |
Member
There was a problem hiding this comment.
Is this change actually needed?
I think I'm seeing the parameters id being computed and set in the call to keyBuilder.forTile(obj).
If you can confirm, please limit this change to what's actually needed for mass tile deletion (also, TMSKeyBuilder.buildParametersId(...) seems to be used only here, so it might no longer be necessary)
Contributor
Author
There was a problem hiding this comment.
This change was needed for some of the integration tests, I will revert it and see if I can fix them another way. Perhaps the expectations are wrong.
| final Iterator<long[]> tileLocations = new AbstractIterator<>() { | ||
| // Create a prefix for each zoom level | ||
| long count = IntStream.range(tileRange.getZoomStart(), tileRange.getZoomStop() + 1) | ||
| .mapToObj(level -> scheduleDeleteForZoomLevel(tileRange, level)) |
Member
There was a problem hiding this comment.
This is going to execute a delete for all tiles in a given zoom level... which is an improvement, if the tile range does not have rangeBounds, of if the rangeBounds did cover the whole gridset area.
But if someone set up the job to remove a specific area (e.g., a city of interest) then the current code would delete everything instead.
To expedite this, I would suggest the following:
- Keep the old code in case there is a tile range that is a subset of the gridset bounds
- Use the new code if the bbox is fully covering the gridset bounds instead.
Contributor
Author
There was a problem hiding this comment.
Added code for bounded deletes. Simplest version applied
Contributor
Author
There was a problem hiding this comment.
Added BoundedS3KeySupplier that reduces the S3ObjectSummaries inspected by scanning the x axis between the bounds.
| private String scheduleDeleteForZoomLevel(TileRange tileRange, int level) { | ||
| String prefix = keyBuilder.forZoomLevel(tileRange, level); | ||
| try { | ||
| s3Ops.scheduleAsyncDelete(prefix); |
Member
There was a problem hiding this comment.
This does the same as deleteByGridsetId, deleteByParametersId and delete(layerName).
This is fine, but we have an event problem. The other three methods inform the listeners that the mass delete is about to happen, while the tile range case would inform tile by tile.
Looking at the changes in S3Ops, I have the impression now all asynch deletes are sending events for single tiles... if so, the listeners may end up recording a change "twice", and thus have disk quota go off synch.
Contributor
Author
There was a problem hiding this comment.
Only delete tile with a bounded delete, because even the bounds are not entered by the user defaults are passed though they will be included in the prefix passed to BulkDelete. BulkDelete will only do the notifications to listeners when it is a bounded delete.
| .filter(timeStampFilter), | ||
| BATCH_SIZE) | ||
| .map(deleteBatchesOfS3Objects) | ||
| .peek(tileListenerNotifier) |
Member
There was a problem hiding this comment.
This is what worries me... I don't see anywhere in the code a distinction between the bulk deletes for layers, parameter ids, gridsets (already given a bulk notification) and the "tile by tile" case.
Contributor
Author
There was a problem hiding this comment.
Consumer<List<S3ObjectSummary>> batchPostProcessor =
possibleBounds.isPresent() ? tileDeletionListenerNotifier : NO_OPERATION_POST_PROCESSOR;
return BatchingIterator.batchedStreamOf(
createS3ObjectStream()
.takeWhile(Objects::nonNull)
.takeWhile(o -> !Thread.currentThread().isInterrupted())
.filter(timeStampFilter),
BATCH_SIZE)
.map(deleteBatchesOfS3Objects)
.peek(batchPostProcessor)
.mapToLong(List::size)
.sum();
Only when bounds present.
Member
There was a problem hiding this comment.
I guess that would work. I don't see it in the diffs, not committed yet?
| } | ||
|
|
||
| @Override | ||
| public boolean hasNext() { |
Member
There was a problem hiding this comment.
As written, hasNext is not idempotent, it's going to consume another batch of source items every time hasNext is called. In theory one should be able to call hasNext as many times as desired, it should be "next" that marks the batch as used and set the stage for another one to be prepared (e.g set the currentBatch as null, and hasNext would create it only if null).
Contributor
Author
There was a problem hiding this comment.
Not 100% sure how the AWS iterator is implemented under the hood. This will return the correct batches and allow hasNext to behave as required, provided the souceIterator supports this.
@Override
public boolean hasNext() {
return sourceIterator.hasNext();
}
@Override
public List<T> next() {
List<T> currentBatch = new ArrayList<>(batchSize);
while (sourceIterator.hasNext() && currentBatch.size() < batchSize) {
currentBatch.add(sourceIterator.next());
}
return currentBatch;
}
| * <p> | ||
| */ | ||
| @Ignore // this test fails very often on the AppVeyor build and frequently on Travis, disabling | ||
| // this test fails very often on the AppVeyor build and frequently on Travis, disabling |
Member
There was a problem hiding this comment.
This comment can be removed, we are not using AppVeyor/Travis any longer and the @ignore has been removed.
Added simplest bounded delete. Added test for bounded delete. Added test to check if TileDeleted events are received when a layer is deleted. There is a race in this test, so it can pass even though tileDeleted is sent.
Added simplest bounded delete. Added test for bounded delete. Added test to check if TileDeleted events are received when a layer is deleted. There is a race in this test, so it can pass even though tileDeleted is sent.
Member
|
@alanmcdade check out the Windows failure: Hum... Appveyor was used to run tests on Windows. If we have an issue there, we might use commons lang to check the operating system and skip tests on that platform (using Assume for example). |
aaime
reviewed
Apr 16, 2025
| @@ -0,0 +1,78 @@ | |||
| package org.geowebcache.s3.streams; | |||
…o supply correct data. Added Copyright to BoundedS3KeySupplier Removed redundant test from integration tests Added an await to the AbstractBlobStoreTest as testDeleteRangeSingleLevel was failing with an aborted BulkDelete without it
… with the release script
Downgrade log messages to warnings as they only have a big impact in tests. The Delete will run again later. Disable testTruncateRespectsLevels in windows environment.
Member
|
Looks good, merging and scheduling for backport |
Collaborator
|
The backport to stderrstdoutTo backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.26.x 1.26.x
# Navigate to the new working tree
cd .worktrees/backport-1.26.x
# Create a new branch
git switch --create backport-1390-to-1.26.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4537f2a74032dd442615ec3cdce8d28bd4ea456c,4b8cb053aaaa773e8bd9e6c16e3594afb71aeaba,e0d205a3b1659447236234dc769756a5b6b020a3,ab44cdd2dabefefccfefa7ced738534eb3a0038a,97b67f83fbaa44689b6ea4449d4c219ab645eebb,449a7c700e74ce1ee567119f052e03082c8e81e1,e2e5d8559a420f085eeab2e48d4c2a8e913c0e0d,039d22e8619393de89c7e72d3bc34db2babc4d59,3d19f675f409f0d27dae3c190bf2f5a66c471ee2,da025363b08ea1f31f31a355b407230aab0e4061,cd722f5dca2543e0db217a8cea2a388750c3f540,8629417d98bc8f1c7a3c5f1c3d72b4a5dbab8a7e,80fc13694d48b4af79cf3013068ac1b675da1628,1314791c64c26ce3960aae736d89b812058432da,6f3a40cebd94fdc07d1b9bf2ec6b1673ab57ee7a,5c678ad6bfbb938d167ad31807e4e9a966a5001c,e4ba07481370eadc45923c02dc59fd90933c9ea4,577ac71bda2af27c60f1f93d36e6e8dbca49f22f,3cbb23fb50e4802555890dc06d24c4dec2d59edb
# Push it to GitHub
git push --set-upstream origin backport-1390-to-1.26.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.26.xThen, create a pull request where the |
Member
|
Ouch, the automatic backport failed (likely due to the merge commit in the original commit list). |
Collaborator
|
The backport to stderrstdoutTo backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.27.x 1.27.x
# Navigate to the new working tree
cd .worktrees/backport-1.27.x
# Create a new branch
git switch --create backport-1390-to-1.27.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4537f2a74032dd442615ec3cdce8d28bd4ea456c,4b8cb053aaaa773e8bd9e6c16e3594afb71aeaba,e0d205a3b1659447236234dc769756a5b6b020a3,ab44cdd2dabefefccfefa7ced738534eb3a0038a,97b67f83fbaa44689b6ea4449d4c219ab645eebb,449a7c700e74ce1ee567119f052e03082c8e81e1,e2e5d8559a420f085eeab2e48d4c2a8e913c0e0d,039d22e8619393de89c7e72d3bc34db2babc4d59,3d19f675f409f0d27dae3c190bf2f5a66c471ee2,da025363b08ea1f31f31a355b407230aab0e4061,cd722f5dca2543e0db217a8cea2a388750c3f540,8629417d98bc8f1c7a3c5f1c3d72b4a5dbab8a7e,80fc13694d48b4af79cf3013068ac1b675da1628,1314791c64c26ce3960aae736d89b812058432da,6f3a40cebd94fdc07d1b9bf2ec6b1673ab57ee7a,5c678ad6bfbb938d167ad31807e4e9a966a5001c,e4ba07481370eadc45923c02dc59fd90933c9ea4,577ac71bda2af27c60f1f93d36e6e8dbca49f22f,3cbb23fb50e4802555890dc06d24c4dec2d59edb
# Push it to GitHub
git push --set-upstream origin backport-1390-to-1.27.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.27.xThen, create a pull request where the |
aaime
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improve the handling of bulk deletions of cached tiles in S3 storage.
The new mechanism will fetch all keys that match a prefix and delete them in batches of 1000.
The prefix has the form <grid-set><format><parameters-id><zoom>. From the right parameters can be omitted to shorten the prefix.
The batching should reduce the number of AWS calls and speed up the process.
Prefetching the keys should eliminate the 404 responses seen previously.
The process has been slightly optimised to deal with a range of zoom levels. The deletion will be limited by the selected zoom range and they will run concurrently if sufficient resources are available.
Still seeing a large increase in resource usage during deletes, the aws client creates a large number of keys and object metadata during this process that are short lived and recycled by the garbage collector.