Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Silently skip NO_ERROR Goaway #935

Closed

Conversation

songy23
Copy link
Contributor

@songy23 songy23 commented Jan 18, 2018

Closes #869.

Based on the discussion in #869, I think it's safe to silently skip status UNAVAILABLE with error code NO_ERROR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 84.123% when pulling 4daf1d0 on songy23:no-error-goaway into 7720668 on census-instrumentation:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 84.068% when pulling 4daf1d0 on songy23:no-error-goaway into 7720668 on census-instrumentation:master.

if (e.getLocalizedMessage()
.startsWith(
"io.grpc.StatusRuntimeException: UNAVAILABLE: HTTP/2 error code: NO_ERROR")) {
continue; // Silently skip NO_ERROR Goaway
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the exporter retry in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hesitant on doing retry in this case, since I think NO_ERROR Goaway indicates server should have received and processed the request with no error. Retrying might generate duplicated stats and increase the burden of server.

@codecov-io
Copy link

codecov-io commented Jan 18, 2018

Codecov Report

Merging #935 into master will decrease coverage by 0.19%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master    #935     +/-   ##
==========================================
- Coverage      81.8%   81.6%   -0.2%     
  Complexity     1032    1032             
==========================================
  Files           169     169             
  Lines          4600    4611     +11     
  Branches        431     434      +3     
==========================================
  Hits           3763    3763             
- Misses          730     741     +11     
  Partials        107     107
Impacted Files Coverage Δ Complexity Δ
...r/stats/stackdriver/StackdriverExporterWorker.java 61.86% <0%> (-6.36%) 13 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc71453...a4990bd. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 84.049% when pulling f06a1b7 on songy23:no-error-goaway into 7720668 on census-instrumentation:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 83.995% when pulling 16afb30 on songy23:no-error-goaway into fc71453 on census-instrumentation:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 83.995% when pulling a4990bd on songy23:no-error-goaway into fc71453 on census-instrumentation:master.

@songy23
Copy link
Contributor Author

songy23 commented Jan 20, 2018

Talked offline. I'll close this PR for now, until I can make sure this failure has no impact on the uploaded stats.

@songy23 songy23 closed this Jan 20, 2018
@songy23 songy23 deleted the no-error-goaway branch May 2, 2018 22:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants