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.

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

Delete objects as part of Graveler #4205

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Conversation

nopcoder
Copy link
Contributor

Optimize multiple object delete operation as part of gateway and open-api, by adding DeleteBatch operation that will perform the multiple delete as part of Graveler.

Notes:

  • That this changes removed the 204 status code from delete objects api, in case of successful or any error it will return 200 and the objects error list will include the specific error

Fix #4203

Optimize multiple object delete operation as part of gateway and
openapi, by adding DeleteBatch operation that will perform the multiple
delete as part of Graveler.
@nopcoder nopcoder added the include-changelog PR description should be included in next release changelog label Sep 20, 2022
@nopcoder nopcoder self-assigned this Sep 20, 2022
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Great progress!

pkg/api/controller.go Show resolved Hide resolved
pkg/graveler/graveler.go Outdated Show resolved Hide resolved
pkg/graveler/graveler.go Show resolved Hide resolved
@@ -2775,3 +2806,20 @@ func NewRepoInstanceID() string {
tm := time.Now().UTC()
return xid.NewWithTime(tm).String()
}

// NewMapDeleteErrors map multi error holding DeleteError to a map of object key -> error
func NewMapDeleteErrors(err error) map[string]error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move it to the errors.go file?

pkg/api/controller_test.go Outdated Show resolved Hide resolved
pkg/gateway/operations/deleteobjects.go Outdated Show resolved Hide resolved
pkg/gateway/operations/deleteobjects.go Outdated Show resolved Hide resolved
pkg/gateway/operations/deleteobjects.go Outdated Show resolved Hide resolved
Comment on lines +136 to +138
delErr := c.Catalog.DeleteEntries(ctx, repository, branch, pathsToDelete)
delErrs := graveler.NewMapDeleteErrors(delErr)
for _, objectPath := range pathsToDelete {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you iterating the pathsToDelete, rather than the actual errors?
What should happen if all deletions in batch are successful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the first part of the function we verified authorization - each file that we do not have access we added an error or added to 'pathsToDelete'.
The list 'pathsToDelete' represents all the files we need to delete (errors will include why we can't delete the rest).
Part of the protocol we need to report (optional - quiet flag) which files we deleted or not.
So going over the files it is to either report a successful delete or the specific/general error why it is not.

@nopcoder nopcoder requested a review from itaiad200 September 21, 2022 08:02
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

LGTM

@nopcoder nopcoder merged commit e7c4923 into master Sep 21, 2022
@nopcoder nopcoder deleted the feature/graveler-delete-keys branch September 21, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use bulk-delete API for recursive delete in lakeFSFS
3 participants