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
181 stars 57 forks source link

Add LWTRetryPolicy interface #213

Closed Lorak-mmk closed 2 months ago

Lorak-mmk commented 3 months ago

RetryPolicy may want to return different decisions from GetRetryType depending on whether query is LWT or not - to prevent executing LWT on non-primary replica. This is not possible with current interface, because GetRetryType doesn't know the query type.

This PR introduces new interface, LWTRetryPolicy. If configured retry policy also implements LWTRetryPolicy, then methods from LWTRetryPolicy will be called for LWT statements instead of methods from RetryPolicy.

There are alternative approaches to this issue:

Lorak-mmk commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

wprzytula commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

dkropachev commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

wprzytula commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

Good catch about this q.SetConsistency.

Lorak-mmk commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

Good catch about this q.SetConsistency.

But in that case it's no longer DowngradingConsistencyRetryPolicy, that's why I thought it would be best to skip it for now. Btw what is the use case for this policy? If someone is fine with using lower consistency level, then why use the higher one at all?

Lorak-mmk commented 2 months ago

@wprzytula failed test is flaky according to @sylwiaszunejko , could you rerun CI?

wprzytula commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

Good catch about this q.SetConsistency.

But in that case it's no longer DowngradingConsistencyRetryPolicy, that's why I thought it would be best to skip it for now. Btw what is the use case for this policy? If someone is fine with using lower consistency level, then why use the higher one at all?

The idea is to use a higher consistency level normally, but as an exception allow lower consistency when higher cannot be immediately obtained because of write/read timeout. This is to cope with spikes of load over short time that would otherwise result in timeouts due to requested consistency not reached in time.

Datastax recommends in their drivers using DowngradingConsistencyRetryPolicy always with logging its decisions turned on, to debug issues caused by its too consistency downgrades.

Lorak-mmk commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

Good catch about this q.SetConsistency.

But in that case it's no longer DowngradingConsistencyRetryPolicy, that's why I thought it would be best to skip it for now. Btw what is the use case for this policy? If someone is fine with using lower consistency level, then why use the higher one at all?

The idea is to use a higher consistency level normally, but as an exception allow lower consistency when higher cannot be immediately obtained because of write/read timeout. This is to cope with spikes of load over short time that would otherwise result in timeouts due to requested consistency not reached in time.

Applications choose a consistency level used that is required for their correctness. If the application is still correct with lower consistency level, then it can use this lower level. If it is not, then downgrading consistency will make the application not correct. Anyway, I don't see a good semantics for LWT for this policy. Imo we should skip this, it's not important to solve now.

wprzytula commented 2 months ago

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

There is. I'm not sure how it should be modify, I'd really like some advice

IMO the only change needed for LWT case is replacing the only occurence of RetryNextHost in GetRetryType with Retry.

AFAIK LWT Queries are run in SERIAL or LOCAL_SERIAL CL, so AttemptLWT should avoid q.SetConsistency( for LWT And as @wprzytula suggested GetRetryType should not go for Retry where it does RetryNextHost

Good catch about this q.SetConsistency.

But in that case it's no longer DowngradingConsistencyRetryPolicy, that's why I thought it would be best to skip it for now. Btw what is the use case for this policy? If someone is fine with using lower consistency level, then why use the higher one at all?

The idea is to use a higher consistency level normally, but as an exception allow lower consistency when higher cannot be immediately obtained because of write/read timeout. This is to cope with spikes of load over short time that would otherwise result in timeouts due to requested consistency not reached in time.

Applications choose a consistency level used that is required for their correctness. If the application is still correct with lower consistency level, then it can use this lower level. If it is not, then downgrading consistency will make the application not correct. Anyway, I don't see a good semantics for LWT for this policy. Imo we should skip this, it's not important to solve now.

See java driver documentation for reference.