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
Add timeout to remote store calls, and adjust name of cache timeout. #18695
Add timeout to remote store calls, and adjust name of cache timeout. #18695
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
help="Timeout value for remote cache lookups in milliseconds.", | ||
) | ||
remote_cache_rpc_timeout_millis = IntOption( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to introduce a DurationOption
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which could take human-readable duration strings and convert them internally to a common format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost certainly, yea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind opening a ticket about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…antsbuild#18695) pantsbuild#16196 moved the cache read timeout down to the network layer, which made it much more accurate. But cache lookups also involve a number of calls to a remote `Store`, which did not have their own timeout. This change adds an RPC timeout for `Store` accesses to allow for retries of tar-pitted remote store RPCs, and adjusts the naming of the `--remote-cache-rpc-timeout-millis` option to make it clear that it applies to all cache operations (including writes).
…(Cherry-pick of #18695) (#18697) #16196 moved the cache read timeout down to the network layer, which made it much more accurate. But cache lookups also involve a number of calls to a remote `Store`, which did not have their own timeout. This change adds an RPC timeout for `Store` accesses to allow for retries of tar-pitted remote store RPCs, and adjusts the naming of the `--remote-cache-rpc-timeout-millis` option to make it clear that it applies to all cache operations (including writes).
#16196 moved the cache read timeout down to the network layer, which made it much more accurate. But cache lookups also involve a number of calls to a remote
Store
, which did not have their own timeout.This change adds an RPC timeout for
Store
accesses to allow for retries of tar-pitted remote store RPCs, and adjusts the naming of the--remote-cache-rpc-timeout-millis
option to make it clear that it applies to all cache operations (including writes).