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

Fix grpclb_end2end_test InitiallyEmptyServerlist #14568

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Fix grpclb_end2end_test InitiallyEmptyServerlist #14568

merged 1 commit into from
Mar 2, 2018

Conversation

dgquintas
Copy link
Contributor

@dgquintas dgquintas commented Mar 1, 2018 •
edited
Loading

After #14507, timing has slightly changed. In the case of the InitiallyEmptyServerlist, subchannels weren't become connected quickly enough to become part of the RR's ready list before the test checked that every backend processed an RPC.

I've further simplified the test, which should only concern itself with verifying that calls are delayed at least until the serverlist has been received.

Fixes #14567

@dgquintas dgquintas changed the title Add cooldown between test rpcs Add cooldown between RPCs in grpclb_end2end_test Mar 1, 2018
@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

@dgquintas dgquintas changed the title Add cooldown between RPCs in grpclb_end2end_test Fix grpclb_end2end_test InitiallyEmptyServerlist Mar 2, 2018
@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

AspirinSJL
AspirinSJL reviewed Mar 2, 2018
View reviewed changes
test/cpp/end2end/grpclb_end2end_test.cc Outdated
@@ -536,10 +534,10 @@ class GrpclbEnd2endTest : public ::testing::Test {
return status;
}

void CheckRpcSendOk(const size_t times = 1) {
void CheckRpcSendOk(size_t times = 1, int timeout_ms = 1000) {
Copy link
Member

Choose a reason for hiding this comment

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

times should still be const? So does timeout_ms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making const a variable passed by value makes no difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BUT, well, it'd be tantamount to declaring any other variable local to the function as const, and I'm usually the one preaching about it, so yeah, let me make them both const.

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link 

[trickle] No significant performance differences

1 similar comment
@grpc-testing
Copy link 

[trickle] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

3 similar comments
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link 

[microbenchmarks] No significant performance differences

@dgquintas dgquintas merged commit c372f19 into grpc:master Mar 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AspirinSJL AspirinSJL AspirinSJL approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@dgquintas @grpc-testing @AspirinSJL