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 an explicit mapping from thrown errors to possible responses #2152

Merged
merged 1 commit into from Jan 30, 2019

Conversation

Projects
None yet
4 participants
@dzajkowski
Copy link
Collaborator

dzajkowski commented Jan 30, 2019

Overview

What this PR does, and why it's needed
Due to the way grpc handles unhandled exceptions we are getting messages that don't mean much (UNKNOWN).
This is a step to make this situation better (but it uses existing responses which is not ideal)

JIRA ticket:

Create it if there isn't one already.
RHAIN-2858

Notes

Optional. Add any notes on caveats, approaches you tried that didn't work, or anything else.

Please make sure that this PR:

Bors cheat-sheet:

  • bors r+ runs integration tests and merges the PR (if it's approved),
  • bors try runs integration tests for the PR,
  • bors delegate+ enables non-maintainer PR authors to run the above.
@ArturGajowy
Copy link
Collaborator

ArturGajowy left a comment

From a bird's eye, this looks like a nice temporary solution. It seems to me, that all communication with the client should be wrapped by a protocol message similar to Either[String, A].

@dzajkowski dzajkowski force-pushed the dzajkowski:RCHAIN-2858/node/UNKNOWN-is-not-a-good-response branch from 91d6a18 to a78bc61 Jan 30, 2019

@dzajkowski

This comment has been minimized.

Copy link
Collaborator Author

dzajkowski commented Jan 30, 2019

bors try

bors bot added a commit that referenced this pull request Jan 30, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 30, 2019

try

Build succeeded

@rabbitonweb
Copy link
Collaborator

rabbitonweb left a comment

approve as workaround solution

@rabbitonweb

This comment has been minimized.

Copy link
Collaborator

rabbitonweb commented Jan 30, 2019

imho this is "strzelanie do komara z armaty" (daily polish lesson for those interested)

changes are local so pulling up typeclass here seems unnecessary. A simple pattern match case would suffice since:

  1. this is temp solution for problem at hand
  2. change is local (1 file changed) so typeclass here seems to big tool for the job

@dzajkowski dzajkowski force-pushed the dzajkowski:RCHAIN-2858/node/UNKNOWN-is-not-a-good-response branch from a78bc61 to 26e7d20 Jan 30, 2019

@dzajkowski

This comment has been minimized.

Copy link
Collaborator Author

dzajkowski commented Jan 30, 2019

bors r+

bors bot added a commit that referenced this pull request Jan 30, 2019

Merge #2152
2152: Add an explicit mapping from thrown errors to possible responses r=dzajkowski a=dzajkowski



Co-authored-by: Dominik Zajkowski <dzajkowski@invisibl.es>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 30, 2019

@bors bors bot merged commit 26e7d20 into rchain:dev Jan 30, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment