Make retry strategies cluster aware

[harunzengin]

Closes #320

[harunzengin]

We're getting the following error on the CI, not sure what it means:

1) test all the datatypes are documented in the guides (DataTypesTest)
Error:      test/integration/datatypes_test.exs:31
     ** (UndefinedFunctionError) function Mix.Project.project_file/0 is undefined or private. Did you mean one of:
     
           * in_project/3
           * in_project/4
     
     code: Mix.Project.project_file()
     stacktrace:
       (mix 1.11.4) Mix.Project.project_file()
       test/integration/datatypes_test.exs:33: (test)

[whatyouhide]

Left a first round of comments, there are a couple of major things to hash out:

1. We don't want to break support for retry strategies on non-cluster connections
2. We don't want to change the signature of RetryStrategy callbacks

[whatyouhide]

Mh, am I wrong or we are removing retrying altogether from non-cluster connections? If so, then we shouldn't do that 🙃

Let's also avoid renaming functions to do_ prefixes, since it's a long-gone convention in Elixir.

[harunzengin]

Ah I thought that when elevating retries to cluster level, we both meant that we don't want Xandra level retries anymore. Is the reason why we don't let the client implement a helper function but to have a retry strategy is to not checkout the Xandra connection pool multiple times? Because that would be the only logical reason to have a retry logic, since everything else can be done with more context with a helper function on the client side.

Otherwise, if we want to have the retry logic on Xandra level as well, maybe we want to have a different callback than Xandra.RetryStrategy? Since the retry/3 function is expected to return a Xandra connection pool, we can't really have the same callback for Xandra.Cluster and Xandra.

[whatyouhide]

Is the reason why we don't let the client implement a helper function but to have a retry strategy is to not checkout the Xandra connection pool multiple times? Because that would be the only logical reason to have a retry logic

I disagree, and I think calling this "the only logical reason" can come off a bit arrogant 😉 Logical reasons to have retry logics are:

1. We would introduce a breaking change by removing them. This might not be a good reason for having introduced them, but alas, it's what we have!
2. It's easier to reuse retry logic modules provided by Xandra or other people. The intention initially was to provide other built-in retry strategies, such as retrying with lower consistency requirements and stuff like that. That never really happened, but still 🙃

Otherwise, if we want to have the retry logic on Xandra level as well, maybe we want to have a different callback than Xandra.RetryStrategy?

As I said in other comments, we can use the options for all of this. We can inject the node to use in the options, and use that in Xandra.Cluster.<...> functions if present.

[harunzengin]

Sorry, didn't mean to be that harsh :) I implemented the requested changes now, with the RetryStrategy callback unchanged as requested. I still feel that the focus should be more on cluster side with this functionality rather than Xandra level though, as it is limited by the functionality of the RetryStrategy callback about what you can and cannot do.

[harunzengin]

@whatyouhide I think this is ready for another pass. I couldn't understand the aformentioned

** (UndefinedFunctionError) function Mix.Project.project_file/0 is undefined or private. Did you mean one of:

error in the CI though.

[whatyouhide]

Why did we add :random here? Can't a retry strategy implement it by itself?

[harunzengin]

This is only for internal use, not exposed to the outside. It's basically a fallback for when a :target_connection doesn't exist.

[harunzengin]

Not sure whether I understood "implement it by itself"

[whatyouhide]

I mean, a retry strategy can implement choosing a random node if :target_connection doesn't exist, right? Unless we have a strong reason, let's just remove this.

[harunzengin]

Yes, exactly, we pop the :target_connection before calling retry_strategy.retryand then do

            |> Keyword.put_new(:target_connection, :random)

after calling retry_strategy.retry, so that if it is empty, we'll be assigning it a random node. This is to have a mechanism to initially select the first node from load balancing strategy.

[harunzengin]

@whatyouhide But using nil instead of :random also works, as in d54ab5b, if that's what you meant

[harunzengin]

@whatyouhide Done. Are we sure that we're supporting Elixir 1.11, since the CI fails and we get the following warning:

warning: the dependency :dialyxir requires Elixir ">= 1.12.0" but you are running on v1.11.4

[whatyouhide]

Yes we don't care about that, we can run Dialyxir only on the latest Elixir, but Xandra itself must support 1.11+ for now. Dialyxir is a dev/test dep, not a prod dep.

[whatyouhide]

Replaced by #335.