-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
deployment: JD improvements #15239
base: develop
Are you sure you want to change the base?
deployment: JD improvements #15239
Conversation
I see you updated files related to
|
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
deployment/environment/devenv/don.go
Outdated
@@ -367,7 +367,8 @@ func (n *Node) CreateJobDistributor(ctx context.Context, jd JobDistributor) (str | |||
func (n *Node) SetUpAndLinkJobDistributor(ctx context.Context, jd JobDistributor) error { | |||
// register the node in the job distributor | |||
err := n.RegisterNodeToJobDistributor(ctx, jd) | |||
if err != nil { | |||
// TODO: check for rpc code = "AlreadyExists" instead | |||
if err != nil && !strings.Contains(err.Error(), "AlreadyExists") { |
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.
so if the node is already registered in JD, we still want to continue?
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.
yeah I'm planning to change it so it's create or update, that way it's idempotent and can be re-ran (it's a total pain to wipe the config on all the nodes if you need to retry)
msg := err.GetMessage() | ||
return "", fmt.Errorf("failed to create feeds manager: %v", msg) | ||
} | ||
return "", fmt.Errorf("failed to create feeds manager: %v", response.GetCreateFeedsManager().GetTypename()) |
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.
can the response be nil at any point here?
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.
I think I found a better way of handling all these: converting to status.Status
then handling the Code
/Message
values: https://github.com/grpc/grpc-go/blob/master/Documentation/rpc-errors.md
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.
actually that won't work, the error responses aren't returned as actual errors... we'll just have to write custom cases
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.
We have custom errors for loops that know how to translate to grpc and still match via errors.Is
No description provided.