scylladb / gocql

Package gocql implements a fast and robust ScyllaDB client for the Go programming language.
https://docs.scylladb.com/stable/using-scylla/drivers/cql-drivers/scylla-go-driver.html
BSD 3-Clause "New" or "Revised" License
188 stars 59 forks source link

Idempotent flag ignored for retries #331

Open pdbossman opened 6 days ago

pdbossman commented 6 days ago

Hello, I was testing goCQL behavior with request shedding enabled. So I set max_concurrent_requests_per_shard: 10

I was testing vanilla insert with value clause.

Initially, requestRetryPolicy was disabled, and I received shedding errors.

I then enabled requestRetryPolicy with ExponentialBackoffRetryPolicy.

    requestRetryPolicy := &gocql.ExponentialBackoffRetryPolicy{
        Min:        100 * time.Millisecond,  // Minimum retry delay
        //Min:        time.Second,  // Minimum retry delay
        Max:        10 * time.Second, // Maximum retry delay
        NumRetries: 10,           // Maximum number of retries
    }

As expected, queries were retried. I still observed shedding in monitoring, but the driver retried the requests with exponential backoff.

I then tested with idempotent set to false.

func insertQuery(session *gocql.Session, ctx context.Context, firstName, lastName, address, pictureLocation string, increment int, logger *zap.Logger) bool {
    //sp := &gocql.SimpleSpeculativeExecution{NumAttempts: 2, TimeoutDelay: 1 * time.Millisecond}
    query := session.Query("INSERT INTO mutant_data (first_name,last_name,address,picture_location) VALUES (?,?,?,?)", firstName, lastName, address, pictureLocation)
    //query.SetSpeculativeExecutionPolicy(sp)
    query.Idempotent(false)
    if err := query.Exec(); err != nil {
        logger.Error("insert mutant_data "+firstName+" "+lastName, zap.Error(err))
    }
}

Problem: Even with Idemotent false, which should block retry - the queries were still retried with exponential backoff policy. It also looks like it might be in general for all retries (speculative and timeout): https://github.com/apache/cassandra-gocql-driver/pull/1809/commits/92b4056c1a125e7094b397cfab8bd8ac8d48f26e

All retry policies should honor the idempotent flag.

tarzanek commented 6 days ago

if we decide to merge upstream, resp. fix this we should consider that there are users who depend on this behaviour - e.g. they retry counter queries with retry policy - so we will need to get them a way to use old behaviour (I expect making sure you can force counter query to not be idempotent and then having above upstream fix will make it fly)

pdbossman commented 6 days ago

if we decide to merge upstream, resp. fix this we should consider that there are users who depend on this behaviour - e.g. they retry counter queries with retry policy - so we will need to get them a way to use old behaviour (I expect making sure you can force counter query to not be idempotent and then having above upstream fix will make it fly)

@tarzanek the flag is settable by the user.
They can just add query.Idempotent(true) now to tell the driver to retry it.

Michal-Leszczynski commented 2 days ago

@pdbossman the scylla-go-driver is a repo for a gocql replacement project that was started, but never finished. This issue should be ceated in the gocql repo.

Michal-Leszczynski commented 2 days ago

Unfortunately, I don't have permissions to transfer this issue there. @Lorak-mmk do you have such permissions, and if so, could you do it?

Lorak-mmk commented 2 days ago

I don't really work with any Go drivers and have no permissions for those repositories. @sylwiaszunejko / @dkropachev may be able to do this.

sylwiaszunejko commented 2 days ago

I don't have permissions to scylla-go-driver do I cannot transfer the issue

mykaul commented 1 day ago

I don't have permissions to scylla-go-driver do I cannot transfer the issue

Done!