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 NamespaceUnavailable serviceerror #194

Merged

Conversation

bergundy
Copy link
Member

Continue the work I started in temporalio/api#481.

Why?

To differentiate between a namespace that's temporary unavailable and not active.
This new failure will be returned with a gRPC unavailable error and should be retried by clients automatically.

@bergundy bergundy requested review from a team as code owners November 21, 2024 01:48
@bergundy bergundy force-pushed the namespace-unavailable-serviceerror branch from c617e5a to 10ebbfd Compare November 21, 2024 01:49
Comment on lines 40 to 41
// Note that other service errors have a private status.Status field, there's no compelling reason to do
// copy that pattern here. The status can and should be computed on the fly in case the
Copy link
Member

Choose a reason for hiding this comment

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

Is there a compelling reason to not copy the pattern? Technically one reason that copying the pattern is that the status object could contain additional details. But that's not likely to happen. Still, I see no good reason not to keep the status object as we always have.

Copy link
Member Author

@bergundy bergundy Nov 21, 2024

Choose a reason for hiding this comment

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

I'm actually thinking of taking it to the other direction and not even keeping the message from the wire. That way if the error is mutated (e.g. a different Namespace string is set, it is serialized correctly).

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't put too much thought into it, just keep the status you're given like other similar errors

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol, I'll just do that to move things along. I wanted to improve what we had, but I can see arguments in both directions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@bergundy bergundy requested a review from cretz November 22, 2024 14:15
}
)

// NewNamespaceUnavailable returns new NamespaceInvalidState error.
Copy link
Contributor

Choose a reason for hiding this comment

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

NamespaceInvalidState error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I should have replaced with /g.

@bergundy bergundy enabled auto-merge (squash) November 27, 2024 19:23
@bergundy bergundy merged commit 0bc7dbc into temporalio:master Nov 27, 2024
3 checks passed
@bergundy bergundy deleted the namespace-unavailable-serviceerror branch November 29, 2024 18:29
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.

3 participants