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

Chunk FindMissingBlobsRequest appropriately #20708

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

tgolsson
Copy link
Contributor

@tgolsson tgolsson commented Mar 22, 2024

Fixes #20674

@jasonwbarnett
Copy link
Contributor

I haven't forgotten about this @tgolsson. I just haven't made time to test this. I'll attempt to get to this no later than end of next week (4/14/2024).

@jasonwbarnett
Copy link
Contributor

@tgolsson it worked!

@tgolsson tgolsson added category:bugfix Bug fixes for released features remote labels May 13, 2024
@tgolsson tgolsson force-pushed the ts/max-message-size-find-missing-blobs branch from 694efe3 to b7d8edc Compare May 13, 2024 16:07
@tgolsson tgolsson marked this pull request as ready for review May 13, 2024 16:17
@tgolsson tgolsson requested a review from huonw May 13, 2024 16:17
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice

docs/notes/2.22.x.md Outdated Show resolved Hide resolved
provider
.list_missing_digests(&mut test_data.into_iter())
.await,
Ok(HashSet::new())
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the test. I think there's a few plausible mistakes that we could introduce in future that won't be caught:

  1. only sending each unique digest once, thus invalidating this test (i.e. if we do the HashSet idea, the list_missing_digests will be working with just one digest and thus only send one request)
  2. skipping the second+ request (either not sending them at all, or not including the results in the final aggregation)
  3. Not sure about this one: if we break this in another way and start sending >4MiB requests, does the StubCAS reject the requests (i.e. does this test fail without the fix)?

I think we can address each of them:

  1. check assert_eq!(cas.request_count(RequestType::CASFindMissingBlobs), 2) so that the test starts failing if that core assumption changes
  2. add two digests that is missing, within each "chunk" (e.g. one at the start and one at the end)
  3. add request size enforcement to StubCAS if not already there (this might cause other tests to fail, so we might want it to be optional until we resolve them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add request size enforcement to StubCAS if not already there (this might cause other tests to fail, so we might want it to be optional until we resolve them

If I remove my fix, the existing test fails.

assertion `left == right` failed
  left: Err("OutOfRange: \"Error, decoded message length too large: found 7000000 bytes, the limit is: 4194304 bytes\"")
 right: Ok({})

@huonw
Copy link
Contributor

huonw commented May 13, 2024

Oh, also, does this fix #20674? Could you update the PR description like "fixes #20674" to have the appropriate auto-close behaviour. Thanks!

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

We've just branched for 2.22, so merging this pull request now will come out in 2.23, please move the release notes updates to docs/notes/2.23.x.md. Thank you!

@huonw
Copy link
Contributor

huonw commented Sep 11, 2024

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

@cburroughs
Copy link
Contributor

I think from the comments this is >90% of the way there and fixes a bug. @tgolsson would you mind dusting it off?

@huonw
Copy link
Contributor

huonw commented Feb 6, 2025

(We're now up to docs/notes/2.26.x.md for release notes.)

@tgolsson tgolsson force-pushed the ts/max-message-size-find-missing-blobs branch from 0890fb9 to bb84987 Compare February 6, 2025 08:56
@tgolsson tgolsson requested a review from huonw February 6, 2025 08:56
@tgolsson tgolsson modified the milestone: 2.24.x Feb 6, 2025
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Really nice, thanks for revisiting it!

@huonw
Copy link
Contributor

huonw commented Feb 6, 2025

(I've edited the Github magic auto-closing incantation of "Fixes #20674" into the PR description, but feel free to revise if that's not correct.)

@tgolsson tgolsson merged commit 2e4e6ff into main Feb 7, 2025
24 checks passed
@tgolsson tgolsson deleted the ts/max-message-size-find-missing-blobs branch February 7, 2025 10:45
@WorkerPants
Copy link
Member

I was unable to cherry-pick this PR; the milestone seems to be missing.

@tgolsson: Please add the milestone to the PR and re-run the Auto Cherry-Picker job using the "Run workflow" button.

🤖 Beep Boop here's my run link

@WorkerPants WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Feb 7, 2025
@tgolsson tgolsson modified the milestones: 2.25.x, 2.23.x, 2.24.x Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-cherry-picking-failed Auto Cherry-Picking Failed category:bugfix Bug fixes for released features needs-cherrypick remote
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remote cache ResourceExhausted/OutOfRange errors
5 participants