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

do shard deletions in batches #321

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

whyrusleeping
Copy link
Collaborator

compaction on rapidly changing repos is slow and 99% of the time is spent in deleteShard. This speeds it up a little bit, from there we can look at what else gets in the way.

@whyrusleeping whyrusleeping force-pushed the feat/compaction-batch-delete branch from 34932e0 to 9499008 Compare September 18, 2023 22:54
Copy link
Collaborator

@ericvolp12 ericvolp12 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +988 to +997
for _, sh := range shs {
if err := cs.deleteShardFile(ctx, sh); err != nil {
if !os.IsNotExist(err) {
return err
}
outErr = err
}
}

return outErr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this bit just to return at least one IsNotExist if any of the shards end up already having been deleted but we don't wanna let that stop us from deleting the others, however if there's some other error we're interested in stopping immediately and returning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exactly, yeah. i want to at least try to delete everything, if we fail because the file is already gone, we continue (but still let our caller know it happened). if we fail for some other error i think it makes sense to bail out immediately

@whyrusleeping whyrusleeping merged commit 5726fa8 into main Sep 19, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants