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

clientv3: Fix parsing of ETCD_CLIENT_DEBUG #14203

Merged
merged 2 commits into from
Jul 13, 2022
Merged

clientv3: Fix parsing of ETCD_CLIENT_DEBUG #14203

merged 2 commits into from
Jul 13, 2022

Conversation

Jille
Copy link
Contributor

@Jille Jille commented Jul 8, 2022

It checked err == nil rather than err != nil.

@ptabor

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022 •
edited
Loading

Codecov Report

Merging #14203 (e3fd58d) into main (20bf49c) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #14203      +/-   ##
==========================================
- Coverage   75.49%   75.25%   -0.24%     
==========================================
  Files         455      455              
  Lines       36859    36859              
==========================================
- Hits        27828    27740      -88     
- Misses       7299     7375      +76     
- Partials     1732     1744      +12
Flag Coverage Δ
all 75.25% <0.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/v3/logger.go 27.77% <0.00%> (ø)
server/etcdserver/api/rafthttp/peer_status.go 87.87% <0.00%> (-12.13%) ⬇️
server/etcdserver/api/rafthttp/peer.go 87.01% <0.00%> (-8.45%) ⬇️
client/v3/leasing/util.go 91.66% <0.00%> (-6.67%) ⬇️
server/etcdserver/api/v3rpc/util.go 74.19% <0.00%> (-6.46%) ⬇️
client/v3/concurrency/mutex.go 61.64% <0.00%> (-5.48%) ⬇️
raft/rafttest/node.go 95.00% <0.00%> (-5.00%) ⬇️
server/etcdserver/api/v3rpc/interceptor.go 73.43% <0.00%> (-4.17%) ⬇️
server/etcdserver/txn/util.go 75.47% <0.00%> (-3.78%) ⬇️
server/proxy/grpcproxy/watch.go 93.64% <0.00%> (-2.90%) ⬇️
... and 17 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 20bf49c...e3fd58d. Read the comment docs.

ahrtr
ahrtr reviewed Jul 8, 2022
View reviewed changes
client/v3/logger.go
@@ -51,7 +51,7 @@ func etcdClientDebugLevel() zapcore.Level {
return zapcore.InfoLevel
}
var l zapcore.Level
if err := l.Set(envLevel); err == nil {
if err := l.Set(envLevel); err != nil {
Copy link
Member

@ahrtr ahrtr Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The change should be good. But the message below is a little confusing (see line 55). Is ETCD_CLIENT_DEBUG really deprecated? cc @serathius

Deprecated env ETCD_CLIENT_DEBUG value. Using default level: 'info'

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 parsed that sentence as the value being deprecated, rather than the setting.

#12786 introduced this code and explicitly brought back ETCD_CLIENT_DEBUG.

Copy link
Member

@ahrtr ahrtr Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't the following message make more sense? @ptabor

Invalid value for environment variable 'ETCD_CLIENT_DEBUG'. Using default level: 'info'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. That would be definitely better.

@ahrtr
Copy link
Member

ahrtr commented Jul 8, 2022

Thanks @Jille for the PR, please sign off the commit,

git commit --signoff --amend

@Jille
clientv3: Fix parsing of ETCD_CLIENT_DEBUG
e3fd58d 
It checked `err == nil` rather than `err != nil`.

Signed-off-by: Jille Timmermans <jille@quis.cx>
ptabor
ptabor approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you.

Please fix the error message as well (as proposed by @ahrtr).

@Jille
Improve error message for incorrect values of ETCD_CLIENT_DEBUG
ac3f913 
Signed-off-by: Jille Timmermans <jille@quis.cx>
ahrtr
ahrtr approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks both @ptabor and @Jille

@ahrtr ahrtr merged commit 16c10d6 into etcd-io:main Jul 13, 2022
@ahrtr
Copy link
Member

ahrtr commented Jul 13, 2022

@Jille Could you backport this PR to 3.5? cc @serathius

@Jille Jille mentioned this pull request Jul 14, 2022
Merged
@Jille
Copy link
Contributor Author

Jille commented Jul 14, 2022

Sure: #14222

@Jille Jille deleted the patch-1 branch July 14, 2022 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptabor ptabor ptabor approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
backport/v3.5
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jille @codecov-commenter @ahrtr @ptabor