Use removeBlobs() for bulk-delete. #39
Conversation
| } catch (ContainerNotFoundException e) { | ||
| result.numberNotFound += 1; | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
We really shouldn't have printStackTrace anywhere in our code.
There was a problem hiding this comment.
Done -- was following the other code in the project (e.g. AuthResource, ExceptionLogger, and IdentityResource).
timuralp
force-pushed
the
bug/use-remove-blobs
branch
from
c96cff5
to
52aa43b
Compare
|
|
timuralp
force-pushed
the
bug/use-remove-blobs
branch
from
52aa43b
to
7c9fe56
Compare
|
👍 |
1 similar comment
|
👍 |
| @@ -151,33 +153,48 @@ public BulkDeleteResult bulkDelete(@NotNull @PathParam("account") String account | |||
| } | |||
|
|
|||
| BulkDeleteResult result = new BulkDeleteResult(); | |||
| Map<String, List<String>> removeBlobsMap = new HashMap<>(); | |||
There was a problem hiding this comment.
Do you want to use a Multimap instead?
There was a problem hiding this comment.
That would work too. I'll change the commit.
We should use removeBlobs() for bulk-delete. The change deletes blobs en-masse. Further, we no longer will perform a HEAD on every blob, as that's fairly expensive. Fixes #38
timuralp
force-pushed
the
bug/use-remove-blobs
branch
from
7c9fe56
to
7ff978e
Compare
| } | ||
| int separatorIndex = objectContainer.indexOf('/'); | ||
| if (separatorIndex < 0) { | ||
| deleteContainers.add(objectContainer.substring(1)); |
There was a problem hiding this comment.
I think you're right, because we already do substring(1) on line 161. I'm not sure how this works right now for containers that start with "/"!
There was a problem hiding this comment.
We are probably deleting a non-existent container which does not generate an error. We should enable/add some swift-tests to check for these behaviors.
|
@timuralp Do we we a path forward on this PR? |
|
@gaul I'll need to reload this into my brain. Will try to check it out over the weekend. |
We should use removeBlobs() for bulk-delete. The change deletes blobs
en-masse. Further, we no longer will perform a HEAD on every blob, as
that's fairly expensive.
Fixes #38