Re-introduce "CSDS Implementation"#25762
Merged
lidizheng merged 3 commits intogrpc:masterfrom Mar 22, 2021
Merged
Conversation
lidizheng
added
lang/c++
release notes: no
Member
|
FYI, there's already been some work on moving the xDS protos into core components internally. See b/169268726 for details. I'll take a look at the CL internally. |
lidizheng
requested review from
gnossen,
jtattermusch and
veblush
as code owners
lidizheng
force-pushed
the
csds-fix
branch
4 times, most recently
from
1ee1fc3 to
0fe0262
Compare
Contributor
Author
|
@markdroth PTALA. I have updated the generic hack with a more specific hack. Also, the internal counterpart needs to be checked-in first. |
markdroth
approved these changes
Mar 19, 2021
Member
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks great! Please make sure Nico approved before merging.
Contributor
Author
|
@nicolasnoble PTAL to see if the added Bazel rule is okay. |
Contributor
Author
|
cl/364351425 is reviewed by Nico. This PR is cherry-picked as cl/364390547. Merging. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Related #25038 #25745
The first CSDS attempt #25038 was rolled back because Envoy protos are not considered "core" components, or included in any components. But technically, we use generated upb files which means we depend on Envoy protos transitively anyway.
The most correct solution is adding Envoy protos to "core" component. But there are 2 blockers:
compatible_withtag "non_prod" is not instrumented for Envoy protos, which means we have to update the entire build chain'scompatible_withtag, including: envoy, protoc-gen-validate, opencensus, googleapis, and part of internal ProtoBuf ("non_prod" is a superset of "gce"/"mobile", but ironically as a compatible tag it isn't compatible with existing tags) (all above packages are compatible with "gce", so there is no technical reason they won't be compatible with "non_prod").With the release date set as next Tuesday, I don't think we can clear this two blockers quick enough.
Thanks to @gnossen. I just learnt that our staging test spawns GCE, pulls gRPC from GitHub, and test the OSS version. This means we can temporarily hide the CSDS feature in google3, without impacting the ability to test CSDS in production and in staging.
The internal counterpart of this PR is cl/363946493.
cl/363758897, the example produced import CL looks like cl/363759740.