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

Merge "byte store" and "action cache" provider options #20115

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Oct 29, 2023

This replaces uses of RemoteCacheProviderOptions with RemoteStoreOptions (previously RemoteOptions): they're very similar, and only separate because they were previously in very different parts of the codebase, but this has changed with #19958.

This makes a few additional changes to make the merging smoother:

  • removes the "capability cell" sharing optimisation, which enabled sharing of the GetCapabilitiesRequest gRPC request result between the CAS and the remote execution providers, if both were being used with the same address. I thought that optimising one-time (on start-up) requests from 2 to 1 wasn't worth the complexity of plumbing it through these more generic options, but can restore it if it is.
  • renames various fields

This does meant that there's a few options that are seemingly irrelevant to the action cache providers, e.g. "batch API size limit" is meaningless. I think this is fine: I think it's fundamental to supporting multiple providers that there'll be a grab-bag of options, which some providers support and others do not.

The commits are individually reviewable.

(This is one point of #19902.)

@huonw huonw added the category:internal CI, fixes for not-yet-released features, etc. label Oct 29, 2023
@huonw huonw force-pushed the huonw/19902-merged-options branch from 64087ad to 7a8f14d Compare October 29, 2023 05:17
@huonw huonw marked this pull request as ready for review October 29, 2023 09:48
@huonw huonw requested review from stuhood, tdyas and tgolsson October 29, 2023 09:48
Copy link
Contributor

@tgolsson tgolsson left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Looks good: thanks!

@huonw huonw enabled auto-merge (squash) October 31, 2023 21:58
@huonw huonw merged commit 0e3523d into main Oct 31, 2023
@huonw huonw deleted the huonw/19902-merged-options branch October 31, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants