Add LWTRetryPolicy interface

[Lorak-mmk]

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:

• Introduce LWTRetryPolicy as another field in Session and Query: requires changes in user code.
• For LWT queries ignore GetRetryType and retry on the same host: it's less configurable

[dkropachev]

Since RetryableQuery and ExecutableQuery are tightly connected to each other, I would suggest to add IsLWT() bool to RetryableQuery and change RetryPolicy.GetRetryType to GetRetryType(RetryableQuery, error) RetryType

Yes, it is going to be a breaking change, but very light in a sence of fixing it and if user is using built-in retrypolicy it is going to be autoresolved.

WDYT?

[sylwiaszunejko]

I am not sure about doing breaking changes. It is a high priority issue and we want to be able to merge it and release as quickly as possible

[Lorak-mmk]

I don't know what is the policy about breaking changes in gocql, because I don't usually work on this driver and I'm not a maintainer. If you want me to I can implement proposed changes.
My thinking with avoiding breaking changes was:

• Afaik Golang itself maintains backwards compatibility, so I assume its expected in the libraries too
• Even without breaking changes customers often stay on older version. If updating requires changes it will be harder to move users to current version
• It's not just incompatibility with our old version, but with upstream gocql too, so it makes migration harder (and possibly merges with upstream too)
• If it is acceptable to introduce a breaking change, then we can do that in the future too, removing LWTRetryPolicy and applying your proposed change to RetryPolicy.

[wprzytula]

Should we really do this unconditionally, even if error returned by the DB signifies unability to serve requests? E.g., ServerError or Overloaded.

[Lorak-mmk]

My thinking was that as long as we have open connections to the node, it may (at some point) perform the request, so the policy can keep trying.
If we loose connection, then the driver will move to the next node.
It seemed safer to avoid LWT contention at all cost.

cc @vladzcloudius

[wprzytula]

ditto

[dkropachev]

There is one more policy left to cover - DowngradingConsistencyRetryPolicy

[Lorak-mmk]

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]

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]

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]

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]

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]

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

[wprzytula]

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]

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]

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.