[C++] Support protobuf native schema #11388
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BewareMyPower
added
doc
BewareMyPower
requested review from
codelipenghui,
jiazhai,
merlimat,
rdhabalia and
sijie
BewareMyPower
changed the title
Contributor
Author
|
Run CI first to expose some test failures because some Dockerfiles may need changes. |
BewareMyPower
changed the title
Contributor
Author
|
Now all tests passed. |
BewareMyPower
added
doc-required
merlimat
approved these changes
Jul 21, 2021
Technoboy-
pushed a commit
to Technoboy-/pulsar
that referenced
this pull request
Jul 22, 2021
* Add C++ ProtobufNativeSchema implementation * Add tests for ProtobufNativeSchema * Fix schema type error and add tests for protobuf native schema * Upgrade pulsar-build image's protobuf to 3.17.1 * Update protobuf dependency version in documents * Add missed comments * Fix CentOS 7 build
1 task
codelipenghui
pushed a commit
that referenced
this pull request
Jul 30, 2021
…#11492) ### Motivation #11388 added support for protobuf native schema in C++ client. The implementation serializes a protobuf generated class' descriptor to a Base64 encoded string. However, it doesn't add the padding characters while the broker side requires the padding characters. Therefore, if the Base64 encoded string needs the padding characters, the broker will fail to deserialize: > com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `byte[]` from String "": Unexpected end of base64-encoded String: base64 variant 'MIME-NO-LINEFEEDS' expects padding (one or more '=' characters) at the end. This Base64Variant might have been incorrectly configured See https://en.wikipedia.org/wiki/Base64#Decoding_Base64_with_padding for the Base64 with padding. ### Modifications - Add the padding `=` characters if the Base64 encoded string's length is not a multiple of four. - Add a `PaddingDemo.proto` to test the above case to ensure that broker can validate the schema string.
Anonymitaet
removed
the
doc-required
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
* Add C++ ProtobufNativeSchema implementation * Add tests for ProtobufNativeSchema * Fix schema type error and add tests for protobuf native schema * Upgrade pulsar-build image's protobuf to 3.17.1 * Update protobuf dependency version in documents * Add missed comments * Fix CentOS 7 build
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…apache#11492) ### Motivation apache#11388 added support for protobuf native schema in C++ client. The implementation serializes a protobuf generated class' descriptor to a Base64 encoded string. However, it doesn't add the padding characters while the broker side requires the padding characters. Therefore, if the Base64 encoded string needs the padding characters, the broker will fail to deserialize: > com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `byte[]` from String "": Unexpected end of base64-encoded String: base64 variant 'MIME-NO-LINEFEEDS' expects padding (one or more '=' characters) at the end. This Base64Variant might have been incorrectly configured See https://en.wikipedia.org/wiki/Base64#Decoding_Base64_with_padding for the Base64 with padding. ### Modifications - Add the padding `=` characters if the Base64 encoded string's length is not a multiple of four. - Add a `PaddingDemo.proto` to test the above case to ensure that broker can validate the schema string.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
It's a feature catchup of #8372, which implemented protobuf native schema in Java client. This PR is to support protobuf native schema in C++ client.
Modifications
Since the protobuf native schema is based on protobuf-v3, we need to upgrade the protobuf to 3.x.y in build/docker/Dockerfile. From #9046 and PIP-75, the patched protobuf 2.4.1 were replaced by lightproto, so this PR removes protobuf.patch and install protobuf 3.17.1 instead of 2.4.1.
In addition, when I installed the
rvmin docker image, it failed to download GPG keys from remote server. So I adopted another way to download keys, see https://rvm.io/rvm/security#alternatives. The updated image has already been uploaded as apachepulsar/pulsar-build:ubuntu-16.04-pb3 so the related scripts are updated as well.For C++ source code, the modifications are:
PROTOBUF_NATIVEschema and make methods ofCommandsrecognize it so that the schema can be serialized to the actualCommandXXXstruct.createProtobufNativeSchemafunction to create a protobuf nativeSchemaInfobased on a protobuf descriptor. It's not included inpulsar/Client.hbecause it depends on protobuf header. If users don't want to use protobuf native schema, they need no extra protobuf dependency.Since the depended protobuf version has been upgraded, the related documents are updated as well.
Verifying this change
This change added tests
ProtobufNativeSchemaTests.Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changes