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.

Sign up for GitHub

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

Jump to bottom

Reduce noise on cancelled barrage subscriptions #1291

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Reduce noise on cancelled barrage subscriptions #1291

merged 1 commit into from
Sep 16, 2021

Conversation

nbauernfeind
Copy link
Member

Fixes #1205

@nbauernfeind nbauernfeind added this to the Sept 2021 milestone Sep 15, 2021
@nbauernfeind nbauernfeind self-assigned this Sep 15, 2021
niloc132
niloc132 approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Request that you verify that kill -9 on the envoy or websocket proxy doesn't result in UNKNOWN/INTERNAL error codes, since those also seem reasonable to not log, or at least not log the exception.

@nbauernfeind
Copy link
Member Author

I tried killing grpc-proxy, and envoy from docker setup, and envoy from non-docker setup. Both only sent CANCEL. I would prefer not to swallow the other status code types unless we know they are normal behavior.

@nbauernfeind nbauernfeind merged commit 95cada5 into deephaven:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niloc132 niloc132 niloc132 approved these changes

Assignees

@nbauernfeind nbauernfeind

Labels
barrage bug Something isn't working grpc NoDocumentationNeeded
Projects
None yet
Milestone
Sept 2021
Development

Successfully merging this pull request may close these issues.

No need to be verbose about cancelled barrage subscriptions
2 participants
@nbauernfeind @niloc132