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

[Do not merge] EDU-3626: Add docs for Nexus circuit breaker #3220

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rodrigozhou
Copy link
Contributor

What does this PR do?

Add docs for Nexus circuit breaker

Notes to reviewers

Comment on lines 149 to 158
### Circuit Breaker {#circuit-breaker}

The circuit breaker kicks in when requests fail with a [retryable error](https://github.com/temporalio/temporal/blob/13d6cd8cf7a4ba0c4660cf98f672bbd645dca3e7/components/nexusoperations/executors.go#L659)
consecutively as it might indicate that the destination (eg: Nexus service to start operation, or
the caller for callback request) is down or unable to process the request. The default behavior of
the circuit breaker is to open after 5 consecutive failed requests. Once in open state, Nexus taskk
will fail early and requests won't be sent to destination. After a minute in open state, it will
change to half-open state, which will allow only 1 request to be made. If the request is successful,
then the circuit breaker changes its state to closed, and allows all requests to pass through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change I made, all other changes was yarn build.

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

I would defer to someone from documentation to go over the grammar and add a section on how users would find out when a destination is down. Specifically mention the BLOCKED state and maybe who what it'd look like when describing a workflow via the CLI.

@fairlydurable fairlydurable changed the title Add docs for Nexus circuit breaker EDU-3626: Add docs for Nexus circuit breaker Dec 2, 2024
Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Approved with comments.

@@ -336,7 +335,7 @@ const workerOptions = {
tuner: {
tunerOptions: resourceBasedTunerOptions,
},
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. No space between brace and next comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What did you mean? Currently, there are no spaces, you want me to add?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just a code style nit. Feel free to ignore.

@fairlydurable
Copy link
Contributor

@prasek This has you listed as a reviewer.

I'm going to unblock the ticket.

@rodrigozhou Please let me know when you're ready to merge.

Copy link
Contributor

@fairlydurable fairlydurable left a comment

Choose a reason for hiding this comment

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

Technical and style issues addressed. Giving @prasek to have a pass if needed.

Unblocking.

@fairlydurable fairlydurable added the blocked-on-release These changes are waiting on a release from another component label Dec 5, 2024
@fairlydurable fairlydurable changed the title EDU-3626: Add docs for Nexus circuit breaker [Do not merge] EDU-3626: Add docs for Nexus circuit breaker Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked-on-release These changes are waiting on a release from another component cross-team tracked-internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants