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

feat: add retry logging #1160

Merged
merged 5 commits into from Jul 31, 2020
Merged

Conversation

stephaniewang526
Copy link
Contributor

Fixes b/160995457

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 30, 2020
@codecov
Copy link

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.

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

LGTM (with a small change request)

@igorbernstein2 igorbernstein2 dismissed their stale review July 31, 2020 19:40

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants