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.

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

protoreflect analysis #146

Closed
lukesneeringer opened this issue Aug 14, 2019 · 5 comments
Closed

protoreflect analysis #146

lukesneeringer opened this issue Aug 14, 2019 · 5 comments
Assignees

Comments

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Aug 14, 2019

@mingzhi Can you offer some history / context on why we chose to use Google protoreflect rather than the jhump one?

We had a discussion about it today and are considering migrating. In particular, I have the following concerns:

  1. The library we are using is in a state of flux.
  2. We have to run on an unreleased version (!) to read annotations.
  3. The library is poorly documented.
  4. The library does not provide a way to move from a proto object to a descriptor object, which makes certain things very hard.
  5. The library is unintuitive to use (examples: .Name() functions that return an object rather than a string, or functions like .Methods() and .Fields() that return custom objects rather than arrays or slices).
  6. The internal and external versions diverge significantly.

We want to ensure that there is not some critical reason we are missing before we make a decision on what to do here.

@mingzhi
Copy link
Contributor

mingzhi commented Aug 14, 2019

I had some trouble importing jhump's libraries into google3. I suggest you try it before making the final decision. Also, ask jhump if he has any plan joining the mentioned Google protoreflect, e.g., implementing protoreflect interfaces.

My understanding is that Golang protobuf, including the generated codes, will all migrate to the mentioned Google protoreflect library inside and outside of Google. And the Golang team has some OKRs targeting this year -- I remembered that they were targeting to release it before GopherCon, July 2019, but unfortunately, they missed the target.

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Aug 16, 2019

Okay, we talked about this in our standing Friday meeting, and we (@jgeewax, @alin04, and myself) made the following decisions:

  • @alin04 is going to attempt to run the jhump protoreflect in google3 and see if she runs into the same issues.
  • If she is able to get the library working successfully, I will refactor the linter.

@alin04
Copy link

alin04 commented Aug 16, 2019

I had some trouble importing jhump's libraries into google3

Curious what troubles you ran into @mingzhi ?

@mingzhi
Copy link
Contributor

mingzhi commented Aug 16, 2019

I tried both protoreflect libraries -- my understanding is that they are competing solutions and both are attempting to be Golang Protobuf v2.

jhump is providing a whole package for protobuf, including parsing, linking, and convenient interfaces for reading and manipulating proto files. As a result, importing those packages into Google3 was a daunting task -- testing data often contain raw and compiled proto files, and their generated Go codes, which might require patching and recompiling not only the Go files but also the testing proto files.

Moreover, by importing jhump's protoreflect, we will own supporting it in Google3. And we will need more comprehensive, integration tests in Google3, since there might be some edge cases, e.g., during resolving/linking proto files. And since we have much more proto files in Google3, the chance of running into some edge cases is much bigger.

Given that Google's protoreflect is supported by golang team, I felt more comfortable using it. But unfortunately, they are actively developing it as well and bringing more breaking changes. Note: they made it public for internal adoption recently, meaning that their interfaces was finally more mature.

@lukesneeringer
Copy link
Contributor Author

Okay, it turns out that someone else imported the jhump protoreflect internally already. Given that, I am going to refactor this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants