-
Notifications
You must be signed in to change notification settings - Fork 434
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
Rename S3 scrubber to storage scrubber #8013
Conversation
fd32b6e
to
e95da66
Compare
3198 tests run: 3056 passed, 0 failed, 142 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
357918b at 2024-06-11T21:41:05.264Z :recycle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code, the scrubber is S3 specific (it creates an s3 client). To my mind it would make more sense to make it generic first and rename it later. I don't mind too much though.
Yeah currently it's S3 specific in the implementation but I have done first steps in an open PR: #7932 We already use internally the new name, and I'd like the current/old name to not proliferate (and we are about to proliferate it further judging https://github.com/neondatabase/cloud/issues/14024 ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no harm in merging this, but doing it now feels a bit awkward to me. In any case, feel free to merge.
The S3 scrubber contains "S3" in its name, but we want to make it generic in terms of which storage is used (#7547). Therefore, rename it to "storage scrubber", following the naming scheme of already existing components "storage broker" and "storage controller".
Part of #7547