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

feat: add retry logging #1160

Merged
merged 5 commits into from Jul 31, 2020
Merged

Conversation

stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Jul 30, 2020

Fixes b/160995457

@google-cla google-cla bot added the cla: yes label Jul 30, 2020
@codecov
Copy link

@codecov codecov bot commented Jul 30, 2020

Codecov Report

Merging #1160 into master will decrease coverage by 0.10%.
The diff coverage is 12.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1160      +/-   ##
============================================
- Coverage     79.07%   78.97%   -0.11%     
- Complexity     1193     1194       +1     
============================================
  Files           205      205              
  Lines          5258     5266       +8     
  Branches        433      436       +3     
============================================
+ Hits           4158     4159       +1     
- Misses          929      935       +6     
- Partials        171      172       +1     
Impacted Files Coverage Δ Complexity Δ
...m/google/api/gax/retrying/BasicRetryingFuture.java 87.25% <12.50%> (-6.37%) 24.00 <1.00> (+1.00) ⬇️

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 b7646a3...7936734. Read the comment docs.

@stephaniewang526 stephaniewang526 requested a review from pmakani Jul 31, 2020
igorbernstein2
igorbernstein2 previously requested changes Jul 31, 2020
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

I dont think this is a good idea.

Logging should be reserved for unexpected errors and for things that callers can do something about. Retries should be considered part of normal operation. I think this will increase support load for service teams unnecessarily.

Can we discuss this offline before merging this?

Copy link
Contributor

@vam-google vam-google left a comment

LGTM (with a small change request)

@igorbernstein2 igorbernstein2 dismissed their stale review Jul 31, 2020

Spoke offline and this seems like the best option in the short term future

@stephaniewang526 stephaniewang526 merged commit 1575715 into googleapis:master Jul 31, 2020
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants