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

Add global context-rpc-timeout option #676

Merged
merged 9 commits into from
Sep 25, 2024

Conversation

yuandrew
Copy link
Contributor

@yuandrew yuandrew commented Sep 23, 2024

What was changed

Added a new global RPC timeout for within a context.

Why?

A slow remote codec can cause timeouts

Checklist

  1. Closes [Feature Request] Heavy remote codecs need longer timeouts #648

  2. How was this tested:

  1. Any docs updates needed?

@yuandrew yuandrew requested a review from cretz September 23, 2024 18:05
@yuandrew yuandrew marked this pull request as ready for review September 23, 2024 18:05
@cretz
Copy link
Member

cretz commented Sep 23, 2024

This needs to be a general context timeout for the whole system IMO (left unset defaults to per-call of 10s). This is not specific to workflow show. Not sure we want to call it a context-timeout, unsure of a better name though.

Note the timeout being set in this PR is a context one that spans many RPC calls. We may be able to call it RPC timeout if we want, but we need to explain that it's not necessarily per RPC, and IMO it should be a top-level option.

@yuandrew
Copy link
Contributor Author

hmm, is context-rpc-timeout too long? That would let folks know it's the rpc timeout from within a context?

@cretz
Copy link
Member

cretz commented Sep 23, 2024

That would let folks know it's the rpc timeout from within a context?

The problem is that "context" is a Go thing and people using the CLI don't care what language it is written in. I think we can probably have a "rpc timeout" though and document that it spans all RPCs on the CLI. Would be open to any other naming too that represents Go context timeout for life of the command without expecting people to even know we're using Go (but a bit afraid of generic "timeout" because it's only for things using the context). Maybe "client timeout"?

@yuandrew yuandrew changed the title Add rpc timeout option to Workflow show Add global context-rpc-timeout option Sep 23, 2024
@yuandrew
Copy link
Contributor Author

we need to explain that it's not necessarily per RPC

Can you explain what this means? Or does this no longer apply now that I moved this to be a global option, and it applies to all RPC calls for the CLI?

temporalcli/commands.go Outdated Show resolved Hide resolved
temporalcli/commands.go Outdated Show resolved Hide resolved
temporalcli/commands.go Outdated Show resolved Hide resolved
temporalcli/commandsgen/commands.yml Outdated Show resolved Hide resolved
temporalcli/commands.go Outdated Show resolved Hide resolved
@yuandrew yuandrew merged commit 9a375eb into temporalio:main Sep 25, 2024
7 checks passed
@yuandrew yuandrew deleted the workflow-show-rpc-timeout branch September 25, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Heavy remote codecs need longer timeouts
3 participants