Skip to content

grid: enabling parallel connect attempts#15779

Merged
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:http3_parallel
Apr 5, 2021
Merged

grid: enabling parallel connect attempts#15779
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:http3_parallel

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Risk Level: n/a (not used in prod)
Testing: unit tests
Docs Changes: n/a
Part of #15649

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

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

Nice!

EXPECT_NE(grid_.second(), nullptr);

// onPoolFailure should not be passed up the first time. Instead the grid
// should wait on the other pool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: missing period

// Test both pools failing
TEST_F(ConnectivityGridTest, FailureTwice) {
// Test both connections happening in parallel and both failing.
TEST_F(ConnectivityGridTest, DoubleFailureParallel) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also add a separate DoubleFailureSerial test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IMO not worth it - the serial code is pretty trivial compared to parallel but I can add in a follow-up if you think it's worth having

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fine by me!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
DavidSchinazi
DavidSchinazi previously approved these changes Mar 31, 2021
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

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

Thanks!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Very cool. (Note I'm just skimming since this is a WIP feature and @DavidSchinazi reviewed), let me know if there is anything in particular you want me to look at).

@alyssawilk alyssawilk merged commit d451814 into envoyproxy:main Apr 5, 2021
@alyssawilk alyssawilk deleted the http3_parallel branch August 18, 2021 13:59
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