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

clientv3: log warning in case of error sending request #11452

Merged
merged 1 commit into from
Dec 20, 2019

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Dec 14, 2019

In watchGrpcStream#run, we should log the (non-nil) error when sending request.

@codecov-io
Copy link

codecov-io commented Dec 14, 2019

Codecov Report

Merging #11452 into master will increase coverage by 0.11%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11452      +/-   ##
==========================================
+ Coverage   64.11%   64.23%   +0.11%     
==========================================
  Files         403      403              
  Lines       38081    38085       +4     
==========================================
+ Hits        24416    24462      +46     
+ Misses      12037    11974      -63     
- Partials     1628     1649      +21
Impacted Files Coverage Δ
clientv3/watch.go 90.81% <75%> (-0.77%) ⬇️
clientv3/balancer/utils.go 53.84% <0%> (-46.16%) ⬇️
auth/options.go 52.5% <0%> (-40%) ⬇️
pkg/transport/timeout_conn.go 60% <0%> (-20%) ⬇️
pkg/netutil/netutil.go 63.11% <0%> (-7.38%) ⬇️
clientv3/leasing/util.go 91.66% <0%> (-6.67%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
clientv3/concurrency/mutex.go 62.16% <0%> (-5.41%) ⬇️
clientv3/leasing/cache.go 87.22% <0%> (-3.89%) ⬇️
clientv3/leasing/txn.go 88.09% <0%> (-3.18%) ⬇️
... and 19 more

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 322c38e...d94547b. Read the comment docs.

@tedyu
Copy link
Contributor Author

tedyu commented Dec 20, 2019

@jingyih
Can you take a look ?

clientv3/watch.go Outdated Show resolved Hide resolved
clientv3/watch.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

Do we need to guard these log lines with if l.lg != nil {?

clientv3/watch.go Outdated Show resolved Hide resolved
clientv3/watch.go Outdated Show resolved Hide resolved
clientv3/watch.go Outdated Show resolved Hide resolved
clientv3/watch.go Outdated Show resolved Hide resolved
@tedyu tedyu force-pushed the watch-grpc-send-err branch 2 times, most recently from d94547b to 0e91877 Compare December 20, 2019 22:53
@tedyu
Copy link
Contributor Author

tedyu commented Dec 20, 2019

@jingyih
Please take another look.

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

LGTM. just one more nit.

clientv3/watch.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

LGTM

@jingyih jingyih merged commit 5adad5e into etcd-io:master Dec 20, 2019
gyuho added a commit that referenced this pull request Aug 14, 2020
…#12187-upstream-release-3.4

Automated cherry pick of #11452 #12187 on release 3.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants