search

mennanov / fieldmask-utils

Protobuf Field Mask Go utils
MIT License
229 stars 26 forks source link

protobuf v1.3.5 support #14

Closed dougdonohoe closed 3 years ago

dougdonohoe commented 3 years ago

We are using fieldmask-utils here at the New York Times for a proof of concept - it's working well so far, except that it required us to bump up to v1.4.2 of protobuf from v1.3.5. This is causing a big performance issue in our use case (marshaling is almost 2-3x slower). My question is whether fieldmask-utils can work with version 1.3.5? Does it rely on anything specific in v1.4.2?

dougdonohoe commented 3 years ago

I forked, changed the dependency and all tests passed. I'm doing this for now in my go.mod file:

replace github.com/golang/protobuf => github.com/golang/protobuf v1.3.5
replace google.golang.org/genproto => google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
mennanov commented 3 years ago

I don't think this package uses anything in particular from the protobuf package v1.4.2: you can look into the https://github.com/mennanov/fieldmask-utils/blob/master/copy.go file to see how it is used. That being said I think it's safe to downgrade to the protobuf v.1.3.5

Changing your local go.mod file is a better way of resolving the issue in my opinion rather than depending on an older version of a dependency in this package. It's also worth running go test all as well (as suggested at https://github.com/golang/go/wiki/Modules#how-to-upgrade-and-downgrade-dependencies) to see if this change causes any new test failures.

You may also take a look at https://github.com/golang/protobuf/issues/1149

dougdonohoe commented 3 years ago

We've gone with the replace approach mentioned above. The only material change we saw was that some sub-structs were empty in 1.4.2 and nil in 1.3.5 (which for our use case was a immaterial change). Thanks for linking to 1149 - I see that other folks have hit the same performance issue.