Skip to content

embed: explicitly define unix proto#9354

Merged
gyuho merged 1 commit intoetcd-io:masterfrom
hexfusion:u_sock
Apr 19, 2018
Merged

embed: explicitly define unix proto#9354
gyuho merged 1 commit intoetcd-io:masterfrom
hexfusion:u_sock

Conversation

@hexfusion
Copy link
Contributor

This is pending grpc/grpc-go#1883

ref: #9340

@hexfusion
Copy link
Contributor Author

Upstream grpc-go fixes will be merged next week allowing unix support to work.

@hexfusion hexfusion force-pushed the u_sock branch 2 times, most recently from b75a018 to 887518c Compare April 10, 2018 00:42
@hexfusion
Copy link
Contributor Author

@gyuho PTAL.

@hexfusion
Copy link
Contributor Author

WARNING: 2018/04/10 10:53:42 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: Error while dialing dial tcp: address unix://localhost:15452000000: too many colons in address"; Reconnecting to {unix://localhost:15452000000 0 }

I need to understand how this can happen, seems like tests are not using patched grpc-go.

@gyuho
Copy link
Contributor

gyuho commented Apr 10, 2018

@hexfusion etcd master still uses old gRPC, so we may see it for awhile :0 Also we need to wait until we upgrade gRPC in master branch including your fix on unix socket.

@gyuho gyuho added this to the etcd-v3.4 milestone Apr 10, 2018
conn, err := grpc.DialContext(ctx, sctx.addr, opts...)

addr := sctx.addr
// explictly define unix network for gRPC socket support
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it simpler?

if sctx.network == "unix" {
    addr = fmt.Sprintf("%s://%s", network, addr)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, all set

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm thanks @hexfusion !

@hexfusion
Copy link
Contributor Author

@gyuho anything else we need here? I plan on improving test coverage in another PR. Thanks!

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.

2 participants