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

Bump vtprotobuf import #914

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Bump vtprotobuf import #914

merged 1 commit into from
Apr 8, 2024

Conversation

howardjohn
Copy link
Contributor

Goes along with envoyproxy/envoy#33365. This will be required to pull in that Envoy change, but it can merge before it.

valerian-roche
valerian-roche previously approved these changes Apr 5, 2024
Copy link
Contributor

@valerian-roche valerian-roche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Out of curiosity, is there a reason to not provide unmarshal in the protobuf generation? It would remain conditioned under the build tag, and it would be quite useful for people implementing services like sds or ext_authz servers to be able to use vtprotobuf also on the unmarshaling path for performance reasons

Goes along with envoyproxy/envoy#33365. This
will be required to pull in that Envoy change, but it can merge before
it.

Signed-off-by: John Howard <[email protected]>
@howardjohn
Copy link
Contributor Author

howardjohn commented Apr 5, 2024

LGTM Out of curiosity, is there a reason to not provide unmarshal in the protobuf generation? It would remain conditioned under the build tag, and it would be quite useful for people implementing services like sds or ext_authz servers to be able to use vtprotobuf also on the unmarshaling path for performance reasons

I wouldn't mind adding it, mostly excluded since it wasn't terribly important to us and there were already concerns about binary size.

This would need a change to envoyproxy/envoy

@howardjohn
Copy link
Contributor Author

@valerian-roche the Envoy PR landed. I think that means the sync job will be broken until this lands

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @howardjohn

@phlax phlax merged commit c6790f3 into envoyproxy:main Apr 8, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

3 participants