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

Fix NullPointerException with unregistered response code #92

Merged
merged 3 commits into from
Jan 30, 2018
Merged

Fix NullPointerException with unregistered response code #92

merged 3 commits into from
Jan 30, 2018

Conversation

spenserpothier
Copy link
Contributor

  • Checks registered response codes before marking
  • Adds a few additional default response codes
  • Misc. GoogleJavaFormat fixes

Fixes #85

* Checks registered response codes before marking
* Adds a few additional default response codes
* Misc. GoogleJavaFormat fixes
Copy link
Contributor

@xjdr xjdr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -89,7 +93,9 @@ private void executeHandler(ChannelHandlerContext ctx, int streamId, Route route
.handle(ctx.channel().attr(XrpcConstants.XRPC_REQUEST).get());
}

xctx.getMetersByStatusCode().get(h1Resp.status()).mark();
Optional<Meter> responseMap =
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really a map, or a response, maybe call it something like statusMeter?

@@ -89,7 +93,9 @@ private void executeHandler(ChannelHandlerContext ctx, int streamId, Route route
.handle(ctx.channel().attr(XrpcConstants.XRPC_REQUEST).get());
}

xctx.getMetersByStatusCode().get(h1Resp.status()).mark();
Optional<Meter> responseMap =
Optional.ofNullable(xctx.getMetersByStatusCode().get(h1Resp.status()));
Copy link
Contributor

Choose a reason for hiding this comment

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

the getMetersByStatusCode() map should be wrapped to return an Optional. You shouldn't have to wrap it here...

* Rename `responseMap` to `statusMeter`
@andyday
Copy link
Contributor

andyday commented Jan 30, 2018

@spenserpothier this can be merged

@spenserpothier spenserpothier merged commit d8f06f4 into Nordstrom:master Jan 30, 2018
spenserpothier added a commit that referenced this pull request Jan 30, 2018
* This commit somehow didn't get pushed to #92.
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.

Exception thrown when missing meter.
4 participants