fix use of remote store when in eager cache mode #11468
Conversation
[ci skip-build-wheels]
|
|
[ci skip-build-wheels]
|
Added an integration test. |
|
@Eric-Arellano / @stuhood: Is there a way to easily trigger an intrinsic from an integration test? Specifically, download_file_to_digest so I can test that the change actually works. |
| @@ -389,14 +389,26 @@ impl Core { | |||
| ) | |||
| .map_err(|e| format!("Could not initialize Store: {:?}", e))?; | |||
|
|
|||
| let store = if (exec_strategy_opts.remote_cache_read || exec_strategy_opts.remote_cache_write) | |||
There was a problem hiding this comment.
Due to the lack of distinct types on Store (which I can imagine how to fix with types, but which I think that we agree is practical for now), this is challenging to review.
One way to help clear this up would be to ensure that we're always passing two values around: the full_store and the local_only_store (names TBD, but consistency is good). That would apply everywhere, but on this line, it would concretely mean having:
let (full_store, local_only_store) = ...
...which might be identical in some cases, or different in some cases.
Does that make sense?
There was a problem hiding this comment.
Passing around two same-typed values seems even more unwieldly to me.
Doing that forces every component receiving two values to have to choose which one makes sense for it. This change seemed more straight-forward to me, the local store is the only Store exposed throughout the code base (other than in remote cache code) when in eager caching mode, otherwise it is the same Store as always which preserves existing semantics.
Combined with the integration test (which necessarily triggers the intrinsic to download pex) demonstrates the fix works.
| GLOBAL_SCOPE_CONFIG_SECTION: { | ||
| "remote_cache_read": True, | ||
| "remote_cache_write": True, | ||
| "remote_store_server": address, |
[ci skip-build-wheels]
Problem
As described in #11455, in remote cache mode with eager fetching, the only accesses to the remote CAS should be through the remote cache code; the remainder of the Pants code base should only see the local store. Intrinsics, such as download_file_to_digest, still have access to the remote CAS currently, which means that any issues with the remote CAS will cause an error instead of being a warning if the access to the remote CAS had occurred through the remote cache code. The intended behavior is for all problems with the remote CAS to be warnings when in remote cache mode.
Solution
In remote cache mode with eager fetching, only expose the local store to most of the Pants code base.
Result
Added an integration test to test that warnings are generated in remote cache mode.
Fixes #11455