Support of keyspace field for BATCH message
#1778
Conversation
worryg0d
force-pushed
the
batch-keyspace-field
branch
from
82b05d3
to
fcd5565
Compare
worryg0d
force-pushed
the
batch-keyspace-field
branch
from
fcd5565
to
6224a0b
Compare
worryg0d
changed the title
keyspace field for BATCH message
absurdfarce
added
the
protocol_v5
conn.go
Outdated
| @@ -1554,6 +1554,10 @@ func (c *Conn) executeBatch(ctx context.Context, batch *Batch) *Iter { | |||
| customPayload: batch.CustomPayload, | |||
| } | |||
|
|
|||
| if c.version > protoVersion4 { | |||
| req.keyspace = c.currentKeyspace | |||
There was a problem hiding this comment.
The purpose of introducing keyspace field in the protocol is not to copy default keyspace that we used when establishing connection, but to be able to override keyspace on per-statement basis. You need to specify keyspace from Batch type.
There was a problem hiding this comment.
I totally agree with you. I added the SetKeyspace field for the Batch to allow overriding keyspace for the specific batch.
cassandra_test.go
Outdated
| t.Skip("keyspace for BATCH message is not supported in protocol < 5") | ||
| } | ||
|
|
||
| err := createTable(session, "CREATE TABLE batch_keyspace(id int, value text, PRIMARY KEY (id))") |
There was a problem hiding this comment.
You should create table in different keyspace, and then do b.keyspace = "foo". Query session.Query("SELECT * FROM batch_keyspace") should also override the default keyspace.
cassandra_test.go
Outdated
| defer iter.Close() | ||
|
|
||
| for i := 0; iter.Scan(&id, &text); i++ { | ||
| if id != ids[i] { |
There was a problem hiding this comment.
Maybe use require.Equal(t, ...).
| texts := []string{"val1", "val2"} | ||
|
|
||
| b := session.NewBatch(LoggedBatch) | ||
| b.Query("INSERT INTO batch_keyspace(id, value) VALUES (?, ?)", ids[0], texts[0]) |
There was a problem hiding this comment.
I think we should also do the test when one of queries also overrides the keyspace.
There was a problem hiding this comment.
As far as I see from the proto 5 spec we can not override the keyspace for the specific query in the Batch, only for the Batch itself.
4.1.7. BATCH
Allows executing a list of queries (prepared or not) as a batch (note that
only DML statements are accepted in a batch). The body of the message must
be:
<type><n><query_1>...<query_n><consistency><flags>[<serial_consistency>][<timestamp>][<keyspace>][<now_in_seconds>]
There was a problem hiding this comment.
I see that my comment might not be understandable. You can simulate hardcoding keyspace in CQL string itself. But, yeah it is not a very useful test indeed.
… that the query should be executed on
|
@lukasz-antoniak Thanks a lot for your feedback! I added some enhancements according to your review. Could you please take a look at this? |
|
Can this be closed in favor of #1822 ? |
|
Hello👋. I don't see any problem of closing this one and others PRs as well 👍 |
This PR adds support for the keyspace field for the BATCH message as part of Native Protocol v5 Support.
This implementation is based on the current implementation for QUERY and EXECUTE messages in gocql.