hashring.LookupN returns ordered results #211
Conversation
|
FWIW, I'm adding this so I can then add ordered LookupN to the router. My service is stateful and push based, so on graceful shutdown I have an interest in iterating through the N servers in the ring. If the forward fails to the first server, go to the next. If that fails, go to the next, etc. |
hashring/hashring.go
Outdated
|
|
||
| // lookup N unique servers from the red-black tree. If we have not | ||
| // collected all the servers we want, we have reached the | ||
| // end of the red-black tree and we need to loop around and inspect the | ||
| // tree starting at 0. | ||
| r.tree.LookupNUniqueAt(n, replicaPoint{hash: hash}, unique) | ||
| r.tree.LookupNUniqueAt(n, replicaPoint{hash: hash}, unique, &orderedUnique) |
There was a problem hiding this comment.
This is an unusual way to work with slices - it's typical to return the modified slice instead of passing pointers. Why pass a ref here?
There was a problem hiding this comment.
The append in findNUniqueAbove replaces the previous slice with a new one at a different address. Is there a way I could accomplish this without using pointers to the slice and updating the pointer?
There was a problem hiding this comment.
You could model this API after append and return the modified slice.
There was a problem hiding this comment.
Not sure I understand what you mean here. I somewhat suspect that what you're suggesting is what I ended up doing in my test code.
hashring/hashring_test.go
Outdated
| @@ -310,7 +310,8 @@ func TestLookupNLoopAround(t *testing.T) { | |||
| ring.AddMembers(members...) | |||
|
|
|||
| unique := make(map[valuetype]struct{}) | |||
| ring.tree.LookupNUniqueAt(1, replicaPoint{hash: 0}, unique) | |||
| orderedUnique := make([]valuetype, 0, 1) | |||
| ring.tree.LookupNUniqueAt(1, replicaPoint{hash: 0}, unique, &orderedUnique) | |||
There was a problem hiding this comment.
This doesn't test the correctness of the code you've added; please add unit tests to cover the new logic.
There was a problem hiding this comment.
I'd love a test for correctness here as well.
There was a problem hiding this comment.
Any suggestions on how to test this given the way the tests are written so far? Naively, I could just iterate a few times and confirm that the returned results for multiple interations of the same call are stable (i.e. same order), but that still wouldn't test that the order is what we expect.
There was a problem hiding this comment.
If I understand this change correctly (debatable), you should be able to assert that (1) the returned slice has the correct length, (2) that the sort is stable, and (3) that the first item is the same as the returned value from Lookup.
@thanodnl may have some ideas for more thorough testing; the only further tests I can imagine would require using the farmhash code directly.
There was a problem hiding this comment.
I just added tests. See what you all think. I did have to remove the optimization that copied the servers because it copied them out of order because it copies from a map. I also considered a straight traversal copy, but that would make the first server in the tree hot.
Essentially, my tests implement the same feature but using the built-in traverseWhile, so the tests and implementation are the same "feature" checking each other.
There was a problem hiding this comment.
Thanks for this fix! I quickly checked how ringpop-node works and there the list is indeed ordered.
There are two things that I'd like to see fixed marked below. I have no strong opinion about slice vs slicepointer but I have to agree it is not something you see often in golang. Hopefully you two can work together in SF time.
When reach consensus don't wait for me to wake up before landing. Functionally it is completely fine with me.
hashring/hashring_test.go
Outdated
| @@ -399,7 +400,7 @@ func (s *ProcessMembershipChangesSuite) TestAddMember0() { | |||
| s.ring.ProcessMembershipChanges([]membership.MemberChange{ | |||
| {After: s.members[0]}, | |||
| }) | |||
| mock.AssertExpectationsForObjects(s.T(), s.l.Mock) | |||
| mock.AssertExpectationsForObjects(s.T(), &s.l.Mock) | |||
There was a problem hiding this comment.
This line seems to makes the tests fail with a panic.
hashring/hashring_test.go
Outdated
| @@ -310,7 +310,8 @@ func TestLookupNLoopAround(t *testing.T) { | |||
| ring.AddMembers(members...) | |||
|
|
|||
| unique := make(map[valuetype]struct{}) | |||
| ring.tree.LookupNUniqueAt(1, replicaPoint{hash: 0}, unique) | |||
| orderedUnique := make([]valuetype, 0, 1) | |||
| ring.tree.LookupNUniqueAt(1, replicaPoint{hash: 0}, unique, &orderedUnique) | |||
There was a problem hiding this comment.
I'd love a test for correctness here as well.
|
Unless there are no more objections, I'm going to land this in an hour. As far as I can tell all issues raised so far have been addressed. |
The previous implementation of LookupN used a map internally to guarantee uniqueness, but when converted to a slice, ordering of servers in the hashring is lost. This diff updates the internal implementation to maintain a pointer to a slice that is appended during the depth-first search of the red-black tree. This ordered result is converted to a slice of strings to return to the caller.
cc/ @yulunli @akshayjshah