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

Allow for configuring a separate remote cache address #15856

Closed
wants to merge 5 commits into from

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Jun 16, 2022

Adds a separate --remote-cache-address. When not set, the --remote-store-address is still used.

Currently, both the CAS service (ByteStreamServer, ContentAddressableStorageServer) and the ActionCache service (ActionCacheServer) are assumed to be hosted at a single --remote-store-address. This is somewhat arbitrary: while it is common to host those service together, it is not required. And while it is also common to host the Execution service on the same host as the remote store/cache, we have a separate --remote-execution-address flag to allow for different deployment models.

… and write)`.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member Author

stuhood commented Jun 16, 2022

Only the top commit is relevant: the rest is #15854.

See discussion of whether the extra flag is necessary over here: #15854 (comment)

As mentioned over there, the most pressing motivation is to allow the integration test of #15850 to use separate CAS and ActionCache instances, without needing to refactor to merge our servers into a unified server. But I think that the change is independently useful.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Maybe edit the help of --remote-store to clarify how this is different? I don't really understand the difference.

@Eric-Arellano
Copy link
Contributor

Also bump that this requires changing our in-repo docs for remote caching.

@stuhood
Copy link
Member Author

stuhood commented Jun 16, 2022

Also bump that this requires changing our in-repo docs for remote caching.

I was holding off on that until I saw a signal that people were in favor of landing the PR at all... if consensus is against adding the flag, then I'd need to take a different approach to testing for #15850.

@Eric-Arellano
Copy link
Contributor

I'm generally skeptical of this PR, but not a hard no. I don't have enough realworld REAPI experience to know if we can expect users using a different server for CAS vs store

@stuhood
Copy link
Member Author

stuhood commented Jun 17, 2022

Consensus was that there wasn't production justification for this. Will merge the StubCAS and StubActionCache instead.

@stuhood stuhood closed this Jun 17, 2022
@stuhood stuhood deleted the stuhood/remote-cache-address branch June 17, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants