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: disable client side grpc log #4875

Merged
merged 1 commit into from
Mar 27, 2016
Merged

clientv3: disable client side grpc log #4875

merged 1 commit into from
Mar 27, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Mar 27, 2016

Fix #4811.

@gyuho
Copy link
Contributor Author

gyuho commented Mar 27, 2016

I ran some simple code of put and get using clientv3 and confirmed that this disables grpc logs when I killed one of the members.

Before

2016/03/26 21:09:30 transport: http2Client.notifyError got notified that the client transport was broken EOF.
2016/03/26 21:09:31 grpc: Conn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:2379: getsockopt: connection refused"; Reconnecting to "localhost:2379"
2016/03/26 21:09:32 grpc: Conn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:2379: getsockopt: connection refused"; Reconnecting to "localhost:2379"
2016/03/26 21:09:32 grpc: Conn.transportMonitor exits due to: grpc: timed out trying to connect
2016/03/26 21:09:33 grpc: Conn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:2379: getsockopt: connection refused"; Reconnecting to "localhost:2379"
2016/03/26 21:09:35 grpc: Conn.resetTransport failed to create client transport: connection error: desc = "transport: dial tcp 127.0.0.1:2379: getsockopt: connection refused"; Reconnecting to "localhost:2379"

After

[None]

xiang90
xiang90 reviewed Mar 27, 2016
View reviewed changes
clientv3/client.go
)

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

user should be able to set logger themselves. (we by default disable gRPC logging)
probably add the logger interface into Config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Will change. Thanks.

@gyuho gyuho force-pushed the clientv3_disable_grpclog branch from 070761a to 4cb191b Compare March 27, 2016 05:06
@gyuho
Copy link
Contributor Author

gyuho commented Mar 27, 2016

@xiang90 Made the logger configurable. PTAL. Thanks.

xiang90
xiang90 reviewed Mar 27, 2016
View reviewed changes
clientv3/client.go
@@ -66,6 +72,8 @@ type Config struct {

// TLS holds the client secure credentials, if any.
TLS *tls.Config

Logger grpclog.Logger
Copy link
Contributor

Choose a reason for hiding this comment

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

doc string?

@gyuho gyuho force-pushed the clientv3_disable_grpclog branch 2 times, most recently from 26627d7 to 271cd81 Compare March 27, 2016 05:23
@gyuho
Copy link
Contributor Author

gyuho commented Mar 27, 2016

@xiang90 Fixed up and added godoc. PTAL. Thanks!

xiang90
xiang90 reviewed Mar 27, 2016
View reviewed changes
clientv3/client.go
@@ -180,6 +189,15 @@ func newClient(cfg *Config) (*Client, error) {
client.Watcher = NewWatcher(client)
client.Auth = NewAuth(client)
client.Maintenance = &maintenance{c: client}
if cfg.Logger == nil {
// same as default std logger
client.logger = log.New(ioutil.Discard, "", 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is not accurate. probably just remove the comment. we should always comment why not what. the code itself is the what part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Just fixed. Thanks!

@gyuho gyuho force-pushed the clientv3_disable_grpclog branch from 271cd81 to 29fccb3 Compare March 27, 2016 05:39
@xiang90
Copy link
Contributor

xiang90 commented Mar 27, 2016

lgtm

@gyuho
Copy link
Contributor Author

gyuho commented Mar 27, 2016

thanks

@gyuho gyuho merged commit 68b38e7 into etcd-io:master Mar 27, 2016
@gyuho gyuho deleted the clientv3_disable_grpclog branch March 27, 2016 05:57
@heyitsanthony heyitsanthony mentioned this pull request Mar 28, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gyuho @xiang90