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.

Sign up for GitHub

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

Jump to bottom

xds: drop xds v2 support #9760

Merged
merged 4 commits into from
Dec 28, 2022
Merged

xds: drop xds v2 support #9760

merged 4 commits into from
Dec 28, 2022

Conversation

YifeiZhuang
Copy link
Contributor

@YifeiZhuang YifeiZhuang commented Dec 17, 2022 •
edited
Loading

larry-safran
larry-safran reviewed Dec 20, 2022
View reviewed changes
Copy link
Contributor

@larry-safran larry-safran left a comment

Choose a reason for hiding this comment

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

Now that we will only have one version of LrsStream, it would simplify things if we eliminated the extra level represented by the abstract class. It would be nice to move all of the logic in LrsStream into LrsStreamV3 and then renamed LrsStreamV3 to LrsStream. Similarly on the tests, if we merged XdsClientImplTestBase and XdsClientImplV3Test it would make the logic clearer.

larry-safran
larry-safran reviewed Dec 20, 2022
View reviewed changes
Copy link
Contributor

@larry-safran larry-safran left a comment

Choose a reason for hiding this comment

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

It looks to me that EnvoyProtoDataTest.java lines 90-123 should be deleted

@YifeiZhuang
Copy link
Contributor Author

Now that we will only have one version of LrsStream, it would simplify things if we eliminated the extra level represented by the abstract class. It would be nice to move all of the logic in LrsStream into LrsStreamV3 and then renamed LrsStreamV3 to LrsStream. Similarly on the tests, if we merged XdsClientImplTestBase and XdsClientImplV3Test it would make the logic clearer.

Thank you. Removing LrsStream abstract sounds fair. Will do in a separate PR.
For the XdsClientImplTestBase I am slightly inclined to keep it, for future usage. But if it causes too much trouble for us to maintain it, we will consider merging them.

larry-safran
larry-safran approved these changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@larry-safran larry-safran left a comment

Choose a reason for hiding this comment

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

LGTM

@YifeiZhuang YifeiZhuang merged commit 3c5c2be into grpc:master Dec 28, 2022
@YifeiZhuang YifeiZhuang deleted the remove-xds-v2 branch December 28, 2022 18:35
sergiitk added a commit to sergiitk/grpc-java that referenced this pull request Jan 3, 2023
@sergiitk
buildscript: drop xDS v2 interop test.
8ebdf03 
This disables xDS v2 support interop test.
xDS v2 support dropped in grpc#9760.

Internal ref cl/499306755
@sergiitk sergiitk mentioned this pull request Jan 3, 2023
Merged
sergiitk added a commit that referenced this pull request Jan 3, 2023
@sergiitk
buildscripts: drop xDS v2 interop test (#9783)
f4cda00 
This disables xDS v2 support interop test.
xDS v2 support dropped in #9760.

Internal ref cl/499306755
@YifeiZhuang YifeiZhuang mentioned this pull request Jan 5, 2023
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@larry-safran larry-safran larry-safran approved these changes

@ejona86 ejona86 Awaiting requested review from ejona86

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YifeiZhuang @larry-safran