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

Add network_latency_ms field to ALTS's NextHandshakeMessageReq. #138

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

matthewstevenson88
Copy link
Contributor

@matthewstevenson88 matthewstevenson88 commented Dec 11, 2023

Already internally-reviewed (see cl/589894002 and cl/589974172).

@ejona86
Copy link
Member

ejona86 commented Dec 12, 2023

(FWIW, it tends to be useful to copy the CL number here so we can follow things a bit easier. Especially further in the future and trying to figure out what we did.)

@matthewstevenson88
Copy link
Contributor Author

(FWIW, it tends to be useful to copy the CL number here so we can follow things a bit easier. Especially further in the future and trying to figure out what we did.)

Sure thing, done. :)

@ejona86
Copy link
Member

ejona86 commented Dec 12, 2023

optional is not allowed? We should able able to do that, although we may find some places that need upgrades. It looks like the CI here is just using too old a protobuf.

optional in proto3 provides presence information, and it looks like you would want that. How much do you care?

@matthewstevenson88
Copy link
Contributor Author

matthewstevenson88 commented Dec 12, 2023

optional is not allowed? We should able able to do that, although we may find some places that need upgrades. It looks like the CI here is just using too old a protobuf.

optional in proto3 provides presence information, and it looks like you would want that. How much do you care?

It's not a big deal in this case, because I only care about the positive values anyway, so "zero" and "not set" are equivalent.

If there's an easy fix to allow optional so that the next person doesn't run into this issue (e.g. maybe just update the @com_google_protobuf dep to the newest version?), I'm happy to do it in this PR or in a follow-up, whatever you prefer.

@ejona86
Copy link
Member

ejona86 commented Dec 12, 2023

I'm happy to do it in this PR or in a follow-up, whatever you prefer.

Thanks for the offer, but I think I have it. We're going to need to merge #123 and then bumping past 21.x required a non-obvious line and bumping to 25 required further changes to bazel. I've fought this before in other circumstances.

@matthewstevenson88
Copy link
Contributor Author

Thanks Eric!

@matthewstevenson88 matthewstevenson88 merged commit cf2eafd into grpc:master Dec 12, 2023
2 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.

2 participants