Skip to content

Add validation and optimise objects.BulkDelete#2597

Merged
mandre merged 3 commits intogophercloud:masterfrom
shiftstack:bulk_delete_objects
Apr 9, 2023
Merged

Add validation and optimise objects.BulkDelete#2597
mandre merged 3 commits intogophercloud:masterfrom
shiftstack:bulk_delete_objects

Conversation

@pierreprinetti
Copy link
Copy Markdown
Member

@pierreprinetti pierreprinetti commented Mar 22, 2023 •
edited
Loading

This PR makes the following changes to objects.BulkDelete:

  • validate that the container name does not contain /;
  • validate that all provided objects are non-empty;
  • optimise how the request body is built.

Note that the function mentions urlencoding in a comment, but there is no urlencoding. I have removed the comment and kept the behaviour. We might want to consider add urlencoding in a follow-up.

WRT the optimisation:

Use bytes.Buffer instead of repeatedly copying strings to compose the request body for objects.BulkDelete.

This PR adds, and in a separate commit, removes the benchmark tests. To execute the benchmark:

git reset --hard f282ec49
cd openstack/objectstorage/v1/objects/testing
go test -bench=.

Locally I get a marginal gain when bulk-deleting only a few objects, and
good gains (to around 3/4 of the time per execution) with large batches:

goos: linux
goarch: amd64
pkg: github.com/gophercloud/gophercloud/openstack/objectstorage/v1/objects/testing
cpu: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
BenchmarkBulkDelete-8                                246           4536244 ns/op
BenchmarkBulkDeleteFewerObjects-8                  11943             97338 ns/op
BenchmarkBulkDeleteWithBuffer-8                      303           3463102 ns/op
BenchmarkBulkDeleteWithBufferFewerObjects-8        11587             92153 ns/op
PASS
ok      github.com/gophercloud/gophercloud/openstack/objectstorage/v1/objects/testing   19.943s

@pierreprinetti
objects: Add some validation to BulkDelete
e855edc 
* Validate that the container name does not contain `/`
* Validate that all provided objects are non-empty
@pierreprinetti pierreprinetti changed the title Bulk delete objects Add validation and optimise objects.BulkDelete Mar 22, 2023
@pierreprinetti pierreprinetti force-pushed the bulk_delete_objects branch 2 times, most recently from 506b894 to 6b6fadf Compare March 22, 2023 17:51
@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Mar 22, 2023 •
edited
Loading

@pierreprinetti I wonder whether it will work with the new versioning system
https://github.com/gophercloud/gophercloud/blob/master/acceptance/openstack/objectstorage/v1/versioning_test.go
If I'm not mistaken, when versioning is enabled and there are multiple versions of one file, you need to delete all the versions.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 23, 2023 •
edited
Loading

Coverage Status

Coverage: 79.06% (-0.02%) from 79.076% when pulling 58b1c67 on shiftstack:bulk_delete_objects into 6e0b5ea on gophercloud:master.

@pierreprinetti
objects: Add optimised BulkDelete function
f282ec4 
Use `bytes.Buffer` instead of repeatedly copying strings to compose the
request body for `objects.BulkDelete`. This commit does not hook the
function to the API; it provides the benchmark tests.

Benchmark with:

```shell
cd openstack/objectstorage/v1/objects/testing
go test -bench=.
```

Locally I get a marginal gain when bulk-deleting only a few objects, and
good gains (to around 3/4 of the time per execution) with large batches:

```plaintext
goos: linux
goarch: amd64
pkg: github.com/gophercloud/gophercloud/openstack/objectstorage/v1/objects/testing
cpu: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
BenchmarkBulkDelete-8                                246           4536244 ns/op
BenchmarkBulkDeleteFewerObjects-8                  11943             97338 ns/op
BenchmarkBulkDeleteWithBuffer-8                      303           3463102 ns/op
BenchmarkBulkDeleteWithBufferFewerObjects-8        11587             92153 ns/op
PASS
ok      github.com/gophercloud/gophercloud/openstack/objectstorage/v1/objects/testing   19.943s
```
@pierreprinetti
Copy link
Copy Markdown
Member Author

pierreprinetti commented Mar 23, 2023

@pierreprinetti I wonder whether it will work with the new versioning system https://github.com/gophercloud/gophercloud/blob/master/acceptance/openstack/objectstorage/v1/versioning_test.go If I'm not mistaken, when versioning is enabled and there are multiple versions of one file, you need to delete all the versions.

I wouldn't know, but this new function is expected to replace the old one with the same exact behaviour. Plus the validations. Do you think the validations would impact versioned files?

@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Mar 23, 2023

@pierreprinetti I don't know. Just thinking out loud. It may happen that this function consumers may face 409 on objects or container deletion :\

stephenfin
stephenfin reviewed Apr 5, 2023
View reviewed changes
openstack/objectstorage/v1/objects/requests.go
@@ -576,15 +576,25 @@ func CreateTempURL(c *gophercloud.ServiceClient, containerName, objectName strin

// BulkDelete is a function that bulk deletes objects.
Copy link
Copy Markdown
Contributor

@stephenfin stephenfin Apr 5, 2023

Choose a reason for hiding this comment

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

Unrelated to this change, but I wonder if we should mention the (configurable) limit of 10,000 objects per request?

Copy link
Copy Markdown
Member Author

@pierreprinetti pierreprinetti Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't able to find a reference to a limit in the API docs. Did you find that in the code of Swift/Ceph?

Copy link
Copy Markdown
Contributor

@EmilienM EmilienM Apr 5, 2023

Choose a reason for hiding this comment

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

I think it's defined by the user in Swift config:

https://github.com/openstack/swift/blob/6d3d4197151f44bf28b51257c1a4c5d33411dcae/etc/proxy-server.conf-sample#L1029-L1034
https://github.com/openstack/swift/blob/e8cecf7fcc1630ee83b08f9a73e1e59c07f8d372/swift/common/middleware/bulk.py#L309

Copy link
Copy Markdown
Member Author

@pierreprinetti pierreprinetti Apr 6, 2023

Choose a reason for hiding this comment

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

wonderful, thank you @stephenfin @EmilienM. I have added that information in the function docs. PTAL

stephenfin
stephenfin previously approved these changes Apr 5, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@stephenfin stephenfin left a comment

Choose a reason for hiding this comment

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

Looks sane to me

EmilienM
EmilienM previously approved these changes Apr 5, 2023
View reviewed changes
@pierreprinetti
objects: Cleanup, remove benchmarks
58b1c67 
This commit replaces the old BulkDelete function with the new one, which
has been measured to be faster.
@pierreprinetti pierreprinetti dismissed stale reviews from EmilienM and stephenfin via 58b1c67 April 6, 2023 12:08
@mandre mandre merged commit e5cad99 into gophercloud:master Apr 9, 2023
@mandre mandre deleted the bulk_delete_objects branch April 9, 2023 19:42
@mandre mandre added this to the v1.4.0 milestone Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EmilienM EmilienM EmilienM left review comments

@mandre mandre mandre approved these changes

@stephenfin stephenfin stephenfin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

6 participants

@pierreprinetti @kayrus @coveralls @mandre @EmilienM @stephenfin