Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retry policy and fix documentation for Cassandra storage backend #10467

Merged

Conversation

byumov
Copy link
Contributor

@byumov byumov commented Nov 30, 2020

Shortly, this PR fixes two problems:

  1. Current documentation for Cassandra backend is incorrect
  2. There is no option to change the default retry policy for cluster now

More details below...

  1. Current documentation says, that default value for connection_timeout is 0, you might think there is no timeout by default, but in fact, if we don't set these options timeout will be 600ms.
    see:

    if connTimeoutStr, ok := conf["connection_timeout"]; ok {
    connectionTimeout, err := strconv.Atoi(connTimeoutStr)
    if err != nil {
    return nil, fmt.Errorf("'connection_timeout' must be an integer")
    }
    cluster.Timeout = time.Duration(connectionTimeout) * time.Second
    }

    cluster.Timeout is not changing somewhere else and get default value:
    Timeout time.Duration // connection timeout (default: 600ms)

  2. If we have a Cassandra cluster with several nodes, we don't want to get an error if one of nodes has gone. For supporting this behavior with gocql(which used in Cassandra backend) we must set one of RetryPolicy for the cluster: https://github.com/gocql/gocql/blob/5913df4d474e0b2492a129d17bbb3c04537a15cd/policies.go#L158
    By default(current behavior) cluster use "retry on same connection" policy. So, if current active node is down client will get an error:
    https://github.com/gocql/gocql/blob/5913df4d474e0b2492a129d17bbb3c04537a15cd/policies.go#L141
    We can easily fix this by using SimpleRetryPolicy (retry for another connection) when creating the cluster.

Also, one new option added to set timeout for the initial connection.

I tried to make changes as little as possible.
This PR doesn't change the behavior of current installations.

@byumov byumov changed the title Add retry policy and fix documentation for Cassandra backend Add retry policy and fix documentation for Cassandra storage backend Dec 2, 2020
@byumov byumov force-pushed the some-improvements-for-cassandra-backend branch from b34bcef to fdcab88 Compare January 10, 2021 18:59
@byumov
Copy link
Contributor Author

byumov commented Feb 25, 2021

@ncabatoff, Hi! Could you take a look, please?

@aphorise
Copy link
Contributor

Enhancement request along with accompanying documentation improvements do seem relevant. Can this be reviewed for release in the near future?

@aphorise aphorise requested a review from taoism4504 as a code owner August 26, 2022 16:03
@aphorise
Copy link
Contributor

Likely related to #15899 - and on merger user on that issue should be advised to retest before closing that issue too.

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@HridoyRoy HridoyRoy mentioned this pull request Aug 30, 2022
@HridoyRoy
Copy link
Contributor

The changelog check is ok -- I triggered the CI run off another branch and PR, so the check is looking for the wrong changelog entry.

@Zlaticanin Zlaticanin self-requested a review August 30, 2022 17:39
Copy link
Contributor

@Zlaticanin Zlaticanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We gave it a test run and it all seems good! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants