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

scrubber: perform manifest check with etag and refactors #10106

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Dec 12, 2024

This adds the etag to remote_storage::ListingObject, and uses it in the scrubber for performing the manifest freshness check added in #10007. This is in response to review giving feedback about the clock precision not being enough to prevent races.

I think the addition of the etag is useful enough as a general change, even without the motivation of wanting to do cheap checks whether there has been a change or not.

Note that the etag check still isn't 100% perfect, and we could get there also by just comparing the manifest, but I think the addition of the etag is useful enough of its own right. I don't know, please write if you think that it's baggage.

As for why the etag check is not perfect: it is fully determined by the content at least on AWS (md5 on AWS, didn't check Azure). So if you change a file from A to B and back to A, the etag will be the original one. Maybe something based on the version ID would be better, but we can't assume to always have it, as sometimes versioning is disabled on the bucket.

Follow-up of #10007

@arpad-m arpad-m requested a review from problame December 12, 2024 00:19
@arpad-m
Copy link
Member Author

arpad-m commented Dec 12, 2024

@problame do you think the ListingObject etag addition is worth it? If not, I'll close this, or remove it and just derive Eq on RemoteTenantManifestInfo.

Copy link

No tests were run or test report is not available

Test coverage report is not available

The comment gets automatically updated with the latest test results
6e6eac6 at 2024-12-12T00:34:38.778Z :recycle:

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.

1 participant