Add support for batches #65
Conversation
proxy/request.go
Outdated
| var hash [16]byte | ||
| copy(hash[:], q) | ||
| if maybeIdempotent, ok := r.client.preparedIdempotence.Load(hash); !ok { | ||
| return false, errors.New("unable to determine if prepared statement in batch is idempotent") |
There was a problem hiding this comment.
If this fails, the proxy logs this error and continues processing the request assuming idempotent == false? It might be worth implementing a mechanism to return UNPREPARED when a cache miss occurs on the proxy (or re-preparing on the fly like the drivers do).
However, repreparing / returning UNPREPARED might be an issue if an error occured with the query parsing when processing the PREPARE message... Retrying in this case is pointless.
There was a problem hiding this comment.
You're right, there's something off here.
It might be worth implementing a mechanism to return UNPREPARED when a cache miss occurs on the proxy (or re-preparing on the fly like the drivers do).
It does do this, but maybe not for batches.
There was a problem hiding this comment.
Ah, never mind. We are caching prepared statements and re-preparing automatically. This was added as part of this PR:
#28
The relevant code is here: https://github.com/datastax/cql-proxy/blob/v0.0.4/proxycore/clientconn.go#L293-L328
There was a problem hiding this comment.
This error case is possible, but is highly unlikely to happen.
There was a problem hiding this comment.
So it's safe to assume that if this particular look up fails then we have to handle it (by assuming idempotent == false) without repreparing because it was probably caused by something that would not get fixed by repreparing?
There was a problem hiding this comment.
I see a better way to do this using the existing prepared cache instead of having yet another map. Trying to think of a way to make it clean-ish.
There was a problem hiding this comment.
So it's safe to assume that if this particular look up fails then we have to handle it (by assuming idempotent == false) without repreparing because it was probably caused by something that would not get fixed by repreparing?
It could happen if the proxy receives a prepared ID that it's never seen before, but I think that would be an application bug.
I see a better way to do this using the existing prepared cache instead of having yet another map. Trying to think of a way to make it clean-ish.
This shouldn't be done, because idempotent state cannot expire; otherwise, the proxy wouldn't be able to determine idempotence without having to have the driver re-prepare the request. This information is tiny and a huge number of variations in prepared queries is an anti-pattern. Also, I discovered a bug, the prepared idempotence cache should be global so that a prepare on one connection propagates its information to other "EXECUTE" requests on other connections. This cannot be tied to each client connection.
Adds support for batches, both the batch CQL native protocol request and the
BEGIN BATCH ... APPLY BATCHCQL query string.This also includes some fixes for retries and adds tests for retries of prepared statements.
Fixes: #55