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

ui: handle errors from unimplemented services #18020

Merged
merged 2 commits into from
Jul 21, 2023
Merged

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Jul 20, 2023

When a request is made to an RPC service that doesn't exist (for example, a cross-region request from a newer version of Nomad to an older version that doesn't implement the endpoint) the application should return an empty list as well.

When a request is made to an RPC service that doesn't exist (for
example, a cross-region request from a newer version of Nomad to an
older version that doesn't implement the endpoint) the application
should return an empty list as well.
@lgfa29 lgfa29 requested a review from philrenaud July 20, 2023 21:18
@lgfa29 lgfa29 added backport/1.4.x backport to 1.4.x release line backport/1.5.x backport to 1.5.x release line backport/1.6.x backport to 1.6.x release line labels Jul 20, 2023
@github-actions
Copy link

github-actions bot commented Jul 20, 2023

Ember Test Audit comparison

main c0fa251 change
passes 1498 1498 0
failures 1 1 0
flaky 0 0 0
duration 000ms 000ms -000ms

@lgfa29 lgfa29 added this to the 1.6.x milestone Jul 20, 2023
const isNotImplemented = errorCodes.includes('501');
const isNotImplemented =
errorCodes.includes('501') ||
error.message.includes("rpc: can't find service");
Copy link
Member

Choose a reason for hiding this comment

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

Note for reviewers: this error comes from the stdlib: https://cs.opensource.google/go/go/+/refs/tags/go1.20.6:src/net/rpc/server.go;l=613

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a change a UI dev* might short-circuit this with an isNotImplemented handler in the future that just checks for 501 in a specific adapter layer. I don't think it's likely (we've always used super so far to inherit from this one) but it would be safer if we eventually returned 501s for these. I don't know if changing the stdlib is at all a likelihood, though!

*it could be me! I forget stuff all the time!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to wrap the RPC error to change the status code. Not a big problem, but I suspect there may be lots of code to change 😅

I opened #18050 to track this.

@tgross tgross merged commit 5d3639f into main Jul 21, 2023
21 checks passed
@tgross tgross deleted the b-ui-fix-x-region-500 branch July 21, 2023 13:16
tgross pushed a commit that referenced this pull request Jul 21, 2023
When a request is made to an RPC service that doesn't exist (for
example, a cross-region request from a newer version of Nomad to an
older version that doesn't implement the endpoint) the application
should return an empty list as well.
jrasell pushed a commit that referenced this pull request Jul 26, 2023
When a request is made to an RPC service that doesn't exist (for
example, a cross-region request from a newer version of Nomad to an
older version that doesn't implement the endpoint) the application
should return an empty list as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.4.x backport to 1.4.x release line backport/1.5.x backport to 1.5.x release line backport/1.6.x backport to 1.6.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants