Upgrade golang protobuf to v1.1.0

[dweitzman]

Also, simplify the protoc call by removing some intermediate tools and steps involving the python grpcio-tools wrapper.
The motivation for changing the build was that I had trouble getting the old build to work. It might just have been me.
This approach is arguably simpler because it involves fewer tools, no temp files, and no calling "sed" on the output.

Signed-off-by: David Weitzman dweitzman@pinterest.com

[dweitzman]

This seems to work for me on OS X and linux, but it's probably a good idea to try running bootstrap & make proto yourself to double-check that it works on your systems as well

[dweitzman]

Related: #3566

[dweitzman]

It seems like the travis / bootstrap image / non-reproducible build situation could make this kind of change awkward.

This sequence of commands seems to work fine for me locally:

./docker/bootstrap/build.sh percona && make docker_base_percona && docker run -ti vitess/base:percona make unit_test

[sougou]

we may have to push a docker image to hub for this to pass.

[michael-berlin]

Do you know why python -m grpc_tools.protoc didn't work for you for the Go gRPC plugin?

The last time I changed this, I intentionally removed the "protoc" binary as dependency because it already comes with the Python gRPC installation if you launch it as Python module.

If I remember correctly, the current code in the master branch should work both on MacOSX and Linux.

If you let me know which error you hit, let's try to fix that and avoid the "protoc" dependency. Alternatively, we need to better document in this CL that there are two "protoc" in play.

Regardless of that, I am fine with getting rid of the temporary files for the Go gRPC plugin.

[dweitzman]

I don't recall the error I hit trying to make grpc_tools work. I'll try to reproduce it later tonight so we can either fix the error or fail to fix it and document reasons for having two copies of protoc.

[michael-berlin]

Was this MacOSX or Linux?

[dweitzman]

Don't remember. I think I tried OS X first and then Linux, but I didn't spend very long since it seemed easier to build something simpler that worked than debug something more complicated that wasn't working (the duplicate copy of protoc wasn't something I was thinking about).

[dweitzman]

When I naively use $(PROTOC_COMMAND) instead of path/to/protoc on OS X, my command is hanging forever without printing any errors.

[sougou]

I've confirmed that the build will (likely) succeed once we push the updated docker images.
However, when I ran make proto, the python files produced a diff. Can you check on that?

I also did a quick benchmark, and I did not see any change in performance. This is surprising. The numbers were so identical that I'm doubting myself. Maybe someone else could run their own and share results?

[derekperkins]

Does it also need a newer version of gRPC?

[dweitzman]

I hadn't tested performance before, but I just wrote a trivial sort of benchmark which I don't claim is good:

import (
	"fmt"
	"testing"

	"github.com/golang/protobuf/proto"

	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
)

var (
	msgToMarshal     *topodatapb.Tablet
	bytesToUnmarshal *[]byte
)

func init() {
	msgToMarshal = &topodatapb.Tablet{
		Hostname: "some_host",
		PortMap: map[string]int32{
			"grpc": int32(12345),
		},
	}

	bytes, err := proto.Marshal(msgToMarshal)
	if err != nil {
		panic(fmt.Sprintf("Failed to marhsal proto: %v", err))
	}
	bytesToUnmarshal = &bytes
}

func BenchmarkProtobufMarshal(b *testing.B) {
	var err error

	for n := 0; n < b.N; n++ {
		_, err = proto.Marshal(msgToMarshal)
		if err != nil {
			b.Error(err)
		}
	}
}

func BenchmarkProtobufUnmarshal(b *testing.B) {
	var err error
	var output topodatapb.Tablet

	for n := 0; n < b.N; n++ {
		err = proto.Unmarshal(*bytesToUnmarshal, &output)
		if err != nil {
			b.Error(err)
		}
	}
}

On my macbook pro, I do see better performance with the upgrade

# With upgrade
$ go test -bench .
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/vt/proto
BenchmarkProtobufMarshal-8     	 1000000	      1004 ns/op
BenchmarkProtobufUnmarshal-8   	 3000000	       563 ns/op
PASS
ok  	vitess.io/vitess/go/vt/proto	3.373s

# Without upgrade
$ go test -bench .
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/vt/proto
BenchmarkProtobufMarshal-8     	 1000000	      1381 ns/op
BenchmarkProtobufUnmarshal-8   	 2000000	       939 ns/op
PASS
ok  	vitess.io/vitess/go/vt/proto	4.359s

Seems consistent-ish across runs.

(Still with the caveat that this benchmark is exercising very little)

[dweitzman]

Seems like it might also be possible to compile the existing codebase against the newer proto library without re-running protoc (or to forget to run govendor sync when trying to move between new vs old in the same client).

# govendor synced to new protobuf version, no other changes in the vitess repo
$ go test -bench .
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/vt/proto
BenchmarkProtobufMarshal-8     	 1000000	      1093 ns/op
BenchmarkProtobufUnmarshal-8   	 2000000	       740 ns/op
PASS
ok  	vitess.io/vitess/go/vt/proto	3.399s

[sougou]

When I pulled this and ran make proto, it regenerated the python files. Can you check if the same thing happens for you? If so, can you check those changes in?

[dweitzman]

Hmm. I tried running "rm py/vtproto/*.proto && make proto" and it generates the exact same python files for me (except py/vtproto/init.py is still missing -- maybe it's hand-written?)

[sougou]

Time to bite the bullet and merge this.