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

Move non-public proto IDL errors to internal api #4011

Merged

Conversation

vytautas-karpavicius
Copy link
Contributor

What changed?
Moved non-public error messages to internal part of proto IDLs.

Why?
Those messages are not communicated out of frontend service. Validated with:

$ cat idls/thrift/cadence.thrift | grep -E "shared.+Error" | awk '{print $2}' | sort -u
shared.BadRequestError
shared.CancellationAlreadyRequestedError
shared.ClientVersionNotSupportedError
shared.DomainAlreadyExistsError
shared.DomainNotActiveError
shared.EntityNotExistsError
shared.InternalServiceError
shared.LimitExceededError
shared.QueryFailedError
shared.ServiceBusyError
shared.WorkflowExecutionAlreadyStartedError

Only those errors are returned by the front-end. Other messages are used when communicating between Cadence server components. Therefore lets move them to internal part of API and do not expose.

This PR also updates:

  • RetryTaskV2Error to wrap start/end event id/version with VersionHistoryItem so that they would get optional semantics. (Either both id/version is set or none of them).
  • Added owner fields to ShardOwnershipLostError. It was forgotten during initial IDLs creation.

@vytautas-karpavicius vytautas-karpavicius requested a review from a team February 23, 2021 10:40
@coveralls
Copy link

coveralls commented Feb 23, 2021

Coverage Status

Coverage increased (+0.02%) to 64.602% when pulling dfe2799 on vytautas-karpavicius:proto-non-public-errors into dfde9ba on uber:master.

Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

LGTM. Reducing public is always good 👍

I don't suppose there are any "X could be moved to Y, it's only used there" linters for proto? Proactive maintenance like this is sorta troublesome to rely on, though I'm always in favor of doing it.

option go_package = "github.com/uber/cadence/.gen/proto/shared/v1;sharedv1";

import "uber/cadence/api/v1/common.proto";
import "uber/cadence/shared/v1/history.proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

seems unused, as it's not part of the generated code

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 actually required for VersionHistoryItem which is in this file.

@vytautas-karpavicius
Copy link
Contributor Author

LGTM. Reducing public is always good 👍

I don't suppose there are any "X could be moved to Y, it's only used there" linters for proto? Proactive maintenance like this is sorta troublesome to rely on, though I'm always in favor of doing it.

I have checked buf rules. There doesn't seems anything like that.

Though in this particular case, it would not help. As error messages are not really attached to rpc methods (unlike thrift). So there would be no way to track them down. For this case I actually used existing thrift IDLs to pull out this information.

@vytautas-karpavicius vytautas-karpavicius merged commit 24509c2 into uber:master Feb 25, 2021
yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021
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