Conversation
…sters Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
…nto add-cluster-extension Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
…-extension Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
… header file Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
In general looks very good. Thanks a ton for adding the integration test. A few questions and small comments.
/wait
Update the cluster slot map. Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Generally looks very nice. Flushing out some more comments. Thank you!
/wait
test/extensions/clusters/redis/redis_cluster_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks looks good. Some remaining nits. Please merge master to fix CI.
/wait
| current_request_ = nullptr; | ||
| if (!current_host_address_.empty()) { | ||
| auto client_to_delete = client_map_.find(current_host_address_); | ||
| if (client_to_delete != client_map_.end()) { |
There was a problem hiding this comment.
It's possible if the remote connection was close and if the client's onEvent got invoked before the onFailure got called. This is not possible with the current implementation, I put it in here just to be defensive.
There was a problem hiding this comment.
OK that's what I thought. Can you remove this error handling and anything similar that can't happen? Than you.
/wait
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this looks great. I looked at the coverage report and you are still missing some error branch coverage. Please add some more tests so that all of that is covered. Thank you!
/wait
Signed-off-by: Henry Yang <hyang@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Nice work. Still missing a bit of coverage on some error/shutdown cases, but will let it slide. Please add in a follow up.
4 participants
Description: Add phase 1 of redis cluster support, in this phase, we'll periodically refresh the cluster topology and add all the masters nodes to the cluster.
Risk Level: Low
Testing: Added unit tests
Docs Changes:
Release Notes: Done
#5697