ringhash: improve test coverage #6072
Comments
|
I would like to contribute, can you assign this to me. |
|
@avinilcodes - I can gladly assign this to you, but right now it seems very ambiguous. Do you think you'll be able to assess the tests in our repo and compare it to the tests in the other repos, or would you need us to help identify the missing things more concretely? |
|
Hi @dfawley , I can work on this. |
|
@shashank-priyadarshi as @dfawley mentioned earlier, do you think you'll be able to assess the tests in our repo and compare it to the tests in other repos, or would you need us help identify the missing things more concretely? |
|
@shashank-priyadarshi / @avinilcodes -- friendly ping! We are almost half way through "Hacktoberfest" 😄. |
|
Sorry, been busy with work lately, couldn't squeeze this in! Hopefull about the weekend! |
|
This lines up well with some work I have in progress on improving ring hash, and such tests would actually help me, so I'm assigning to me. |
|
There are more tests to be written :) |
Follow up to grpc#7271 to fix grpc#6072. This adds a dozen more end to end tests. There are tests that I did not port, specifically: - TestRingHash_TransientFailureSkipToAvailableReady was flaky when I ported it, so I removed it while investigating. - TestRingHash_SwitchToLowerPriorityAndThenBack was also flaky, I also removed it while investigating. - TestRingHash_ContinuesConnectingWithoutPicksOneSubchannelAtATime, I'm not sure we implement this behavior, and if we do, it's not working the same way as in c-core, where the order of subchannel connection attempts is based on the resolver address order rather than the ring order. I will follow up with fixes for each one of the remaining tests.
Follow up to grpc#7271 to fix grpc#6072. This adds a dozen more end to end tests. There are tests that I did not port, specifically: - TestRingHash_TransientFailureSkipToAvailableReady was flaky when I ported it, so I removed it while investigating. - TestRingHash_SwitchToLowerPriorityAndThenBack was also flaky, I also removed it while investigating. - TestRingHash_ContinuesConnectingWithoutPicksOneSubchannelAtATime, I'm not sure we implement this behavior, and if we do, it's not working the same way as in c-core, where the order of subchannel connection attempts is based on the resolver address order rather than the ring order. I will follow up with fixes for each one of the remaining tests.
Follow up to grpc#7271 to fix grpc#6072. This adds a dozen more end to end tests. There are tests that I did not port, specifically: - TestRingHash_TransientFailureSkipToAvailableReady was flaky when I ported it, so I removed it while investigating. - TestRingHash_SwitchToLowerPriorityAndThenBack was also flaky, I also removed it while investigating. - TestRingHash_ContinuesConnectingWithoutPicksOneSubchannelAtATime, I'm not sure we implement this behavior, and if we do, it's not working the same way as in c-core, where the order of subchannel connection attempts is based on the resolver address order rather than the ring order. I will follow up with fixes for each one of the remaining tests.
|
This work uncovered #7363. |
I just noticed that the amount of tests that we have for the ringhash LB policy is woefully short compared to what C-core and Java have. We should add more tests, which exercise different scenarios.
C-core tests are here: https://github.com/grpc/grpc/blob/master/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc
Java tests are here: https://github.com/grpc/grpc-java/blob/master/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerTest.java
The text was updated successfully, but these errors were encountered: