-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
C# Proto2 feature : Finale #5936
C# Proto2 feature : Finale #5936
Conversation
46049aa
to
3de9ba0
Compare
3de9ba0
to
85bf822
Compare
We're open for review @anandolee. I'll continue to add tests in places I see lacking but it's all nearly there. Feel free to point out anywhere you feel needs more tests. cc: @jskeet if you want to review as well Is there anything else we need to do for proto2 support like docs? |
Yes, we also need to update the docs as well But this can be done later after the support is released |
I'm absolutely not going to have time to review, I'm afraid :( |
I will review once I have a bit of free time. |
Hey all, I'm working on a project at the moment that has a mixture of
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in still in progress of reviewing (it's a lot of code), but I have a few initial comments.
@ObsidianMinor can you also resolve the conflict? |
1142181
to
393fd09
Compare
Add comment explaining IExtensionMessage instability
f17435f
to
bdc6cd1
Compare
I merged both this PR and the the docs #6499. @anandolee when is the planned branch cut / release date for 3.10? |
Any idea why this want mentioned in the 3.10.0rc1 release notes? Did it not make it on? Or is it being "secretly" released to allow for testing before it's official? |
@rafi-kamal are #6499 and #5936 part of the v3.10.x branch? If so, we should update the release notes. If not, seems like the PRs haven't made it on time for the v3.10.x branch cut (and they will be in the next release). |
Unfortunately the branch cut happened about a week ago, before the PRs were merged. It will be included in the next release (ETA: 6 weeks), unless this is something critical and requires cherry-picking. |
@ObsidianMinor given the release happened only a few days back can you cherry pick your changes into current release branch? I'm trying to implement a GRPC External Auth service for Envoy in C# and I'm currently blocked since a number of protos which Envoy uses are in proto2 version. And I'm sure there are many like me who are waiting for this feature for quite a long time. |
@ravgupms If I worked at Google, I might. But I don't, so I can't. Maybe we could create a 3.11 beta branch with the cherry-picked changes and put up a 3.11 beta version of Google.Protobuf on nuget so people could test the proto2 features. Could that work? @anandolee @jtattermusch |
@ObsidianMinor that would be great. Thanks! |
We could cherry-pick this PR and #6499 (the corresponding docs) into 3.10.x branch, but it would be a very large cherry-pick (this PR is 34 commits), so even though I understand that folks are waiting for this feature and i'd like to get it tested sooner than later, backporting such a large (non-critical) change is not something we usually do. I'm open to backporting into 3.10.x branch if @rafi-kamal is ok with that (as an exception to the normal process to facilitate early testing). Releasing 3.11.x is not a good option because that would require cutting the 3.11.x ahead of schedule and that's not a good idea for process reasons (the branch involves all other languages too). |
I agree with @jtattermusch, this is a pretty large change and I would prefer to release it within our normal release schedule instead of cherry-picking it to 3.10. |
Thanks for the awesome work! Any idea when 3.11 will be released? |
Thank you so much ObsidianMinor for driving this! This unblocks us and saves us from migrating some huge protos to proto3! 👍 😆 |
This wraps up all the previous feature PRs. Changes include:
When pulled, this PR will fix numerous proto2 feature issues, including: #5759, #4591, #4407, #3487, #2991