Add consistent hashing to the client

[CodingAnarchy]

This is a significant refactor to add a consistent ring hashing algorithm to the Client, so instead of a single connection to a Memcached node, it instead creates connections to a vector of node DSNs and manages those in a Ring struct.

I bumped the version of the the crate to 0.3.0 as this behavior is significantly incompatible with the existing Client implementation in how it is instantiated, as it expects a vector of DSNs instead of a single one. The existing Client methods have all been replicated in the new structure, though, so this will be relatively transparent except on creating the struct.

The Node class is simply the old Client class with a rename and an additional field to track the DSN in the struct.

I changed the tests to work against a mocked out TCP server, with a transparent fake of the Memcached protocol. This made it easier to test in the unit test whether the expected calls were being made, but comes with the downside of not validating against actual Memcached nodes. An integration test to do this additional testing will likely be necessary to add as a follow up.

Testing in a staging environment (or more than one, if possible) is highly recommended here.

[adampetro]

Have we considered leaving the existing Client and allowing the Ring to be used as an alternative that composes multiple Clients?

[adampetro]

What's the reasoning behind using the newtype pattern instead of aliases? Most of the cases I can think of crates offering result types, they are typically aliases of std::result::Result, for example anyhow::Result and std::io::Result

[CodingAnarchy]

I thought of newtype first. I'm not sure if a type alias will work for the implementation of Extend, etc. but I can try to see if it does here. I agree it would probably be cleaner than the newtype pattern if I can get it working. I'll take a look tomorrow when I have some time.

[adampetro]

I think you might be able to do the equivalent of what you're doing already with collect through this FromIterator implementation

[CodingAnarchy]

@adampetro I looked into this briefly, and the issue with a type alias works well for something like anyhow::Result, where the Error type is used to make the result less generic, it doesn't work here where we need to implement Extend for the stream::iter to be collected. When I swapped this newtype out for a type alias, the compiler (correctly) errors on applying a trait from outside my crate to a type from outside my crate.

[adampetro]

But do you need to implement Extend? Does this FromIterator implementation do what you need?

[CodingAnarchy]

Extend<Self::Item> is a trait bound required by futures::stream::StreamExt#collect, but the standard Result iterator doesn't provide it (or Default, the other requirement) because of the generics on the type.

[CodingAnarchy]

I went this way with futures::stream in order to do multiple network calls in an async concurrent fashion. I understand the newtype pattern here does feel pretty clunky and strange, though.

[adampetro]

Ah, I see now, StreamExt::collect requires Extend.

Can we use a single generic type instead of the three? Something like:

struct CollectibleError<T>(Result<Vec<T>, Error>);

impl<T> Default for CollectibleError<T> {
    fn default() -> Self {
        Self(Ok(Vec::new()))
    }
}

impl<T> Extend<Result<T, Error>> for CollectibleError<T> {
    fn extend<I: IntoIterator<Item = Result<T, Error>>>(&mut self, iter: I) {
        if self.0.is_err() {
            // If an error has already occurred, don't bother continuing
            return;
        }

        for item in iter {
            match item {
                Ok(value) => self.0.as_mut().unwrap().push(value),
                Err(e) => {
                    self.0 = Err(e);
                    return;
                }
            }
        }
    }
}

impl<T> Extend<Result<Vec<T>, Error>> for CollectibleError<T> {
    fn extend<I: IntoIterator<Item = Result<Vec<T>, Error>>>(&mut self, iter: I) {
        if self.0.is_err() {
            // If an error has already occurred, don't bother continuing
            return;
        }

        for mut item in iter {
            match item {
                Ok(ref mut value) => self.0.as_mut().unwrap().append(value),
                Err(e) => {
                    self.0 = Err(e);
                    return;
                }
            }
        }
    }
}

[CodingAnarchy]

Made it a generic type in e1a1403, and put it in its own src/collectible_result.rs file.

[adampetro]

This feels very integration-test-like. Should we move to tests/integration_test.rs or something like that?

[nickamorim]

Could we use maglev for consistent hashing instead? We already have a bulk of this implemented in a closed source client implementation that I can point you to in DM

[CodingAnarchy]

@nickamorim I agree maglev would be an improvement over the more traditional hashing algorithm here, but I think it would require too much of a change for me to implement currently. The main goal here was to unblock us on our current service to be able to scale up past one node, and I personally don't have the time to do that refactor currently. Does it make sense as a follow-on that you or someone else could do that after this PR is merged/released?

[CodingAnarchy]

Have we considered leaving the existing Client and allowing the Ring to be used as an alternative that composes multiple Clients?

I went with this rename approach because the Client in other programming languages (e.g. dalli in Ruby, memcached in NodeJS) uses this same abstraction and so that makes this more predictable for developers coming from other languages. I think we can still expose the Node struct for those who want a direct 1:1 connection to a server themselves.

[nickamorim]

Does it make sense as a follow-on that you or someone else could do that after this PR is merged/released?

Certainly

[dwdking]

.