Protobuf 3 support #615

lb034582341 · 2023-04-05T15:14:57Z

Hello, first time contributor here. Following #584, here's a first draft of the migration to protobuf 3.2. It's a work-in-progress and we'll surely need to iterate to find a good solution.

I haven't gated the changes behind a feature yet (as requested here). I'm not exactly sure how to do this yet.

Also, it seems that for protobuf 3, the functionality of the compiler/ folder has been factored out in the protobuf-codegen crate. Do we want to keep the folder and call the crate, or update the documentation to directly use the crate ?
It's been temporarily removed because it doesn't compile with protobuf 3.2.

The diff is large but only Protobuf 3.2: syntax changes contains meaningful changes (and most of them are trivial).

I have ran tests + clippy successfully.

Feedback is welcome, thanks !

lb034582341 · 2023-04-12T10:23:59Z

Hello, just checking in to see if there's interest in pushing this :) thanks

BusyJay · 2023-04-13T10:04:18Z

Thanks for your contributions! Sorry for the delay, I will take a look in following 3 days.

lb034582341 · 2023-04-13T10:16:19Z

No rush at all on my side! Knowing that it might move forward at a later point is good for me. thanks

BusyJay · 2023-04-17T07:11:51Z

I haven't gated the changes behind a feature yet

From what I can see from this PR, there are three type of changes:

API change
We can introduce a compatible layer in grpc-rs so that no matter 2.x or 3.x is used, we can still using the same API sets. For example, v.field = Some(v).into() should work for both 2.x and 3.x, we don't need to change that; CodedInputStream::from_buffered_reader and CodedInputStream::from_buf_read don't work with each other, we need to introduce a function to invoke diffferent methods when different feature is enabled.
Generated code change
Unfortunately, we can do little thing about it. We may need to generate files for both versions and check in to the version control.
Compiler change
We may also introduce a compatible layer and only expose necessary common APIs. The tricky part here is TiKV project has customized the compiler to provide functionalities like log redact. So we may need to keep using protobuf for v2 and use protobuf-codegen only for v3.

lb034582341 · 2023-05-10T15:08:04Z

Hi BusyJay, sorry for the long wait. Thanks for the suggestions, they have been implemented.

The upgrade is gated behind a new feature protobufv3-codec which behaves like protobuf-codec. It is mutually exclusive with it, and also prost. Just a heads-up, you are probably aware that this might not be recommended, but I followed the logic of the existing code.

No changes have been made to the defaults. To build/test with protobuf v3:
cargo test --features protobufv3-codec --no-default-features

As discussed a lot of changes come from the generated files for v3, which can be ignored. This commit regroups the changes to the various Cargo.toml, and this commit contains the actual rust code changes.

cargo test and cargo clippy have been ran successfully.

thanks for your time!

health/src/service.rs

interop/src/client.rs

src/codec.rs

compiler/src/codegen.rs

lb034582341 · 2023-05-26T07:54:47Z

(Force-push for signoff, sorry)

Thanks a lot @mgeisler for the review!

lb034582341 · 2023-06-12T08:33:22Z

If the code is too verbose, one option is to refactor some of the protobuf2- and protobuf3-specific getters and setters using macros. I could investigate this if there's interest

lb034582341 · 2023-06-16T09:44:40Z

Hello, I apologize for the ping... It's been a month, I'm just wondering what to do with my proposed changes ? Thank you

lb034582341 · 2023-06-28T08:08:08Z

Sorry to insist @BusyJay, is there any way forward for this ? Someone else I could bother?
This is the last crate holding the whole Android codebase on protobuf 2.
Thank you

benchmark/src/bench.rs

proto/src/proto.rs

BusyJay · 2023-07-10T12:15:11Z

interop/src/server.rs

                }
                // A workaround for timeout_on_sleeping_server test.
                // The request only has 27182 bytes of zeros in payload.
                //
                // Client timeout 1ms is too short for grpcio. The server
                // can response in 1ms. To make the test stable, the server
                // sleeps 1s explicitly.
+
+                #[cfg(feature = "protobuf-codec")]
                if req.get_payload().get_body().len() == 27182


You can move this condition to last so that don't need to repeat all other conditions.

Is this what you have in mind ?

tests-and-examples/tests/tests.rs

tests-and-examples/examples_protobufv3/route_guide/util.rs

src/error.rs

src/codec.rs

tests-and-examples/examples/route_guide/client.rs

tests-and-examples/Cargo.toml

proto/src/util.rs

interop/src-protobufv3/bin/client.rs

lb034582341 · 2023-07-14T15:36:26Z

Done. Since the previous presubmit failed due to linting errors, I took the opportunity to clean this up too. Please let me know if this doesn't belong in this PR

lb034582341 · 2023-07-18T16:46:45Z

The last CI error doesn't seem related to my changes, and seems fixed on main. Maybe it has been fixed since ?

BusyJay · 2023-07-19T02:13:30Z

Yes, please solve conflicts.

lb034582341 · 2023-07-19T09:03:35Z

Done, ty

lb034582341 · 2023-07-20T08:38:59Z

OK, CI-wise we should be good (although some tests seem flaky): https://github.com/lb034582341/grpc-rs/actions/runs/5600803677
All these commits are a bit messy. Would you like me to squash them all, or do you prefer to keep the full history ?

BusyJay · 2023-07-20T09:06:03Z

Would you like me to squash them all

Don't worry, they will be squashed when it's merged.

benchmark/src/worker.rs

BusyJay · 2023-07-26T02:44:54Z

@lb034582341 Hi, we may release a new version soon. It would be better if this PR is included to next release. Do you think it can catch the release date (before next week)?

lb034582341 · 2023-07-26T12:50:03Z

Yes, that'd be great.

lb034582341 · 2023-07-28T14:42:41Z

NB: The CI failed due to flakiness, it passed here: https://github.com/lb034582341/grpc-rs/commits/master

Signed-off-by: Ludovic Barman <ludovicb@google.com>

lb034582341 · 2023-08-03T14:57:40Z

Sorry about messing the history, one commit wasn't signed off and DGO was blocking the CI.
Despite the force push, I didn't change what you already reviewed.

Simple things first, I addressed your 3 comments in 8be9ef5, 22e7c0e and 610032f

I added support for v3 in compiler 40c10a8. I removed the previous code that was wrong (was never compiled due to a feature error). Now this simply calls the rust_protobuf crate. Finally, maybe we want to discard this commit, and keep compiler/ mostly about pb2 and prost. xtask can call the rust_protobuf crate directly. Please let me know what you think.

The comment about xtask required more changes:

Changed xtask to output pbv3 code 2867cd2 .
-- I refactored some code common for v2, v3 and prost into new methods;
-- I didn't manage to find the same plugin mechanism on rust_protobuf so I'm using the compiler/ code to generate the _grpc.rs even for protobuf_v3, this works.
-- I added a renaming ::protobuf:: -> ::protobuf_v3:: to match the imports
-- Nit: I'm not sure why the code in remove_protobuf_version_constraint() was there in the first place. I wonder if now that we generate both v2 and v3 code separately, maybe there's no need for it ? I left it there for now.

Finally, I simplified health/ in 648b815 and regenerated all files in the last commit

Signed-off-by: Ludovic Barman <ludovicb@google.com>

compiler/src/codegen.rs

health/Cargo.toml

compiler/Cargo.toml

Signed-off-by: Ludovic Barman <ludovicb@google.com>

BusyJay

Rest LGTM

compiler/Cargo.toml

compiler/src/codegen.rs

health/Cargo.toml

health/tests/health_check.rs

Signed-off-by: Ludovic Barman <ludovicb@google.com>

health/tests/health_check.rs

Signed-off-by: Ludovic Barman <ludovicb@google.com>

lb034582341 · 2023-08-08T13:34:43Z

I've included your changes, thanks for the review. CI passed here https://github.com/lb034582341/grpc-rs/actions/runs/5796423100

BusyJay · 2023-08-09T06:07:10Z

Thank you!

lb034582341 · 2023-10-02T08:19:30Z

@lb034582341 Hi, we may release a new version soon. It would be better if this PR is included to next release. Do you think it can catch the release date (before next week)?

Hi @BusyJay, is there anything I can do to help with the release ?
The reason for this is that I need a new release to get these changes to Android open source project, I can't upgrade otherwise.
Thanks!

BusyJay · 2023-10-07T02:12:22Z

It was already released in crates.io. I just forgot to update github release page. And the page was just updated.

lb034582341 · 2023-10-09T07:26:04Z

Ah, right. Thanks a lot!

lb034582341 changed the title ~~Protobuf 2 -> 3~~ Protobuf 3 support May 10, 2023

lb034582341 marked this pull request as ready for review May 10, 2023 15:08