session: prepare now prepares on all nodes #266
Conversation
Kulezi
force-pushed
the
pp/prepare-fix
branch
4 times, most recently
from
736459f to
92d9bb2
Compare
Kulezi
requested review from
Choraden,
Michal-Leszczynski,
avelanarius and
piodul
There was a problem hiding this comment.
While I think it is a worthwhile change, it shouldn't declare that it fixes #249. AFAIK #249 points out that the protocol specification technically allows different nodes to use different IDs for the same prepared statement. This PR still works under this assumption, it's just that prepared statements are prepared up-front on all nodes.
IMO it's fine to fix this later. All other drivers that I know, including the Java driver, currently seem to work under that assumption, so I doubt the current behavior will be changed soon by either Cassandra or Scylla.
session.go
Outdated
| // Find first result that succeeded. | ||
| for i := range nodes { | ||
| if resErr[i] == nil { | ||
| return Query{session: s, |
There was a problem hiding this comment.
Formatting looks weird here. Is that intentional?
| if c.cfg.Keyspace == "" { | ||
| // We don't know anything about the keyspace, fallback to non-token aware query. | ||
| return c.NewQueryInfo(), nil | ||
| } |
There was a problem hiding this comment.
Is this change necessary for the purpose of this PR (preparing statements on all nodes)? If not, then perhaps it should be sent as a separate PR, or - at least - as a separate commit?
There was a problem hiding this comment.
Creating the test i encountered a missing keyspace error that "" was not found in system_schema.keyspaces, currently the driver always assumes it's querying the default session keyspace which needs to be changed with #257, right now the driver doesn't save info about keyspace from prepared results, hence i needed this workaround. I prefer to do this all with #257.
I moved the workaround to another commit.
session_integration_test.go
Outdated
| func TestPrepareIntegration(t *testing.T) { | ||
| defer goleak.VerifyNone(t) | ||
|
|
||
| cfg := DefaultSessionConfig("", "192.168.100.100:9042") |
There was a problem hiding this comment.
Use the TestHost constant instead of the hardcoded address.
| if _, err := q.Exec(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
I guess the sleep is here in order to simulate a "wait for schema agreement" operation? I suggest placing a TODO comment here so that it will be easier to find the workaround-sleeps when somebody gets to implement it.
|
Applied changes suggested by @piodul, also removed leaking goroutines from TLS integration test caused by unclosed session. |
Prepare now prepares on all nodes, once per node, as sources suggest:
https://github.com/apache/cassandra/blob/adcff3f630c0d07d1ba33bf23fcb11a6db1b9af1/doc/native_protocol_v4.spec#L738-L740
https://github.com/scylladb/scylladb/blob/4d24097b4b86d484159c011f46b9e43268d342d1/transport/server.cc#L941-L950
Also made the driver fallback from token aware policy to round robin
if both session keyspace and query keyspace are unspecified, this is a temporary fix until #257 is resolved.