[xds] Fallback implementation #36145

eugeneo · 2024-03-18T22:02:49Z

markdroth

This looks like a good start! Haven't had a chance to review the tests yet but wanted to send you my comments on the code.

Please let me know if you have any questions. Thanks!

src/core/ext/xds/xds_client.cc

src/core/ext/xds/xds_client.h

src/core/ext/xds/xds_client.cc

markdroth

Here are my comments on the tests.

Please let me know if you have any questions. Thanks!

test/cpp/end2end/xds/xds_server.h

test/cpp/end2end/xds/xds_end2end_test_lib.h

test/core/memory_usage/memory_usage_test.cc

test/cpp/end2end/xds/xds_fallback_end2end_test.cc

test/cpp/end2end/xds/BUILD

eugeneo

Thank you for reviews. I am still adding test cases but the core code is ready for review.

src/core/ext/xds/xds_client.cc

src/core/ext/xds/xds_client.h

src/core/ext/xds/xds_client.cc

test/cpp/end2end/xds/xds_fallback_end2end_test.cc

test/core/memory_usage/memory_usage_test.cc

test/cpp/end2end/xds/BUILD

PiperOrigin-RevId: 617335127

…36154) As per [gRFC A71](https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md). Closes grpc#36154 COPYBARA_INTEGRATE_REVIEW=grpc#36154 from eugeneo:xds-bootstrap-servers-env-var aada3e7 PiperOrigin-RevId: 617579604

markdroth

This is moving in the right direction!

Please let me know if you have any questions. Thanks!

src/core/ext/transport/binder/java/io/grpc/binder/cpp/BUILD

src/core/ext/xds/xds_client.cc

test/cpp/end2end/xds/xds_fallback_end2end_test.cc

test/cpp/end2end/xds/xds_end2end_test_lib.h

src/core/ext/xds/xds_client.cc

markdroth

This is getting closer!

PLMK if you have any questions. Thanks!

src/core/ext/xds/xds_client.cc

test/core/xds/xds_client_test.cc

eugeneo

Thanks for the feedback!

test/core/xds/xds_client_test.cc

markdroth

The tests are better but still need some work. And there was one comment from my previous review pass that you didn't address.

PLMK if you have any questions. Thanks!

test/core/xds/xds_client_test.cc

src/core/ext/xds/xds_client.cc

eugeneo

Thanks for the comments. Addressed.

test/core/xds/xds_client_test.cc

markdroth

Just one remaining comment. Feel free to merge after addressing.

markdroth · 2024-04-09T16:07:24Z

test/core/xds/xds_client_test.cc

  resource = watcher->WaitForNextResource();
+  EXPECT_THAT(GetServerConnections(), ::testing::ElementsAre(::testing::Pair(


Please move this down to line 4146, so that it's not right in between getting the resource from the watcher and checking the resource's value.

Done. Thanks!

[xds] Fallback implementation

3a6c21e

eugeneo added the release notes: no Indicates if PR should not be in release notes label Mar 18, 2024

eugeneo requested a review from markdroth March 18, 2024 22:02

eugeneo assigned markdroth Mar 18, 2024

github-actions bot added lang/c++ lang/core labels Mar 18, 2024

grpc-checks bot added the bloat/low label Mar 18, 2024

Automated change: Fix sanity tests

04594ce

grpc-checks bot added per-call-memory/decrease per-channel-memory/neutral labels Mar 18, 2024

markdroth reviewed Mar 19, 2024

View reviewed changes

wip: tests progress

02722f6

markdroth reviewed Mar 19, 2024

View reviewed changes

eugeneo and others added 6 commits March 19, 2024 13:50

fixup: review #1

7ccdb2e

fixup: code review 2

1a23799

fixup: more review comments

54efc66

Merge remote-tracking branch 'grpc/master' into fallback-review

2b5566e

Automated change: Fix sanity tests

6f4674c

Automated change: Fix sanity tests

512dc9e

grpc-checks bot added per-call-memory/neutral and removed per-call-memory/decrease labels Mar 19, 2024

fixup: this was a bug.

d132309

eugeneo commented Mar 20, 2024

View reviewed changes

eugeneo and others added 4 commits March 20, 2024 12:53

wip

1e1dcd9

Internal change

9d1f1cd

PiperOrigin-RevId: 617335127

fixup: bugfix

b99ed3b

markdroth reviewed Mar 20, 2024

View reviewed changes

eugeneo added 2 commits March 21, 2024 11:26

fixup: before review

d6b89be

fixup: codereview

67dac82

Merge remote-tracking branch 'grpc/master' into fallback-review

42bbe04

grpc-kokoro removed kokoro:run labels Apr 2, 2024

grpc-checks bot added per-call-memory/decrease and removed per-call-memory/increase labels Apr 2, 2024

fixup: memory leak

bf4425b

grpc-checks bot added per-call-memory/increase and removed per-call-memory/decrease labels Apr 2, 2024

markdroth reviewed Apr 3, 2024

View reviewed changes

fixup: memory leak

1aff3ba

eugeneo commented Apr 5, 2024

View reviewed changes

fixup: oops!

4a5868e

grpc-checks bot added per-call-memory/neutral and removed per-call-memory/increase labels Apr 5, 2024

markdroth reviewed Apr 6, 2024

View reviewed changes

fixup: review comments

177060e

eugeneo commented Apr 8, 2024

View reviewed changes

fixup: review comments

53f29d9

grpc-checks bot added per-call-memory/increase bloat/medium and removed per-call-memory/neutral bloat/low labels Apr 8, 2024

markdroth approved these changes Apr 9, 2024

View reviewed changes

fixup: review comment

4871c0b

grpc-checks bot added bloat/low and removed bloat/medium labels Apr 9, 2024

markdroth approved these changes Apr 9, 2024

View reviewed changes

copybara-service bot closed this in 4e735be Apr 9, 2024

eugeneo deleted the fallback-review branch April 9, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[xds] Fallback implementation #36145

[xds] Fallback implementation #36145

eugeneo commented Mar 18, 2024 •

edited by markdroth

Loading

markdroth left a comment

markdroth left a comment

eugeneo left a comment

markdroth left a comment

markdroth left a comment

eugeneo left a comment

markdroth left a comment

eugeneo left a comment

markdroth left a comment

markdroth Apr 9, 2024

eugeneo Apr 9, 2024

		resource = watcher->WaitForNextResource();
		EXPECT_THAT(GetServerConnections(), ::testing::ElementsAre(::testing::Pair(

[xds] Fallback implementation #36145

[xds] Fallback implementation #36145

Conversation

eugeneo commented Mar 18, 2024 • edited by markdroth Loading

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth Apr 9, 2024

Choose a reason for hiding this comment

eugeneo Apr 9, 2024

Choose a reason for hiding this comment

eugeneo commented Mar 18, 2024 •

edited by markdroth

Loading