Support for result_metadata_id in PREPARED and EXECUTE messages
#1782
Conversation
worryg0d
force-pushed
the
prepared-statement-metadata-id
branch
from
860880b
to
b4e0726
Compare
absurdfarce
added
the
protocol_v5
conn.go
Outdated
| preparedID: info.id, | ||
| params: params, | ||
| customPayload: qry.customPayload, | ||
| preparedMetadataID: info.request.id, |
There was a problem hiding this comment.
This is definitely not correct. You should send here <result_metadata_id> received from PREPARE response, not the <id> of PREPARE response.
There was a problem hiding this comment.
Following part of specification has not been implemented:
If a RESULT/Rows message reports
changed resultset metadata with the Metadata_changed flag, the reported new
resultset metadata must be used in subsequent executions.
There was a problem hiding this comment.
Good catch! Thanks a lot. Also, I added the described mechanism. Now the driver receives the RESULT/ROWS response it updates the prepared stmt meta and result_metadata_id in the stmtsLRU cache and re-run query execution.
frame.go
Outdated
| @@ -1604,23 +1611,31 @@ type writeExecuteFrame struct { | |||
|
|
|||
| // v4+ | |||
| customPayload map[string][]byte | |||
|
|
|||
| // v5+ | |||
| preparedMetadataID []byte | |||
There was a problem hiding this comment.
I would consider following naming convention from protocol specification, so this would be result_metadata_id.
There was a problem hiding this comment.
Also good catch! Thanks! I renamed it no resultMetadataID
frame.go
Outdated
| @@ -952,6 +955,10 @@ func (f *framer) parsePreparedMetadata() preparedMetadata { | |||
| // TODO: deduplicate this from parseMetadata | |||
| meta := preparedMetadata{} | |||
|
|
|||
| if f.proto > protoVersion4 { | |||
There was a problem hiding this comment.
<id> field of PREPARE response is already present in V4. Here we are missing reading of <result_metadata_id>.
There was a problem hiding this comment.
I'm sorry for the misunderstanding here. The id field in preparedMetadata doesn't represent <id> in the specification, but result_metadata_id.
Also, the naming is terrible here, which leads to a big misunderstanding.
I moved parsing of the result_metadata_id from this method to the parseResultPrepared method, because the result_metadata_id field is a part of the PREPARED message, not its metadata.
| @@ -127,3 +127,27 @@ func TestFrameReadTooLong(t *testing.T) { | |||
| t.Fatalf("expected to get header %v got %v", opReady, head.op) | |||
| } | |||
| } | |||
|
|
|||
| func Test_framer_writeExecuteFrame_preparedMetadataID(t *testing.T) { | |||
There was a problem hiding this comment.
We need an integration test to validate <result_metadata_id> logic. Details of why it was included can be found in ticket CASSANDRA-10786.
There was a problem hiding this comment.
I totally agree with you here. We should run tests over C* to ensure the logic works fine. However, I struggled with triggering C* to return a <new_metadata_id> by changing the table columns number, but C* responded with UNPREPARED error message, not the RESULT/ROWS.
Could you please give me some advices on how to trigger this mechanism?
There was a problem hiding this comment.
Can you merge all those V5 pull requests into one branch and one PR? I have a test nearly ready, but wanted to double test it.
There was a problem hiding this comment.
@lukasz-antoniak Yes, here it is 1882
… result metadata updating mechanism in the stmts cache in order to avoid stmt re-preparing.
|
Can this be closed in favor of #1822 ? |
This PR provides support for the
result_metadata_idfield in PREPARED response and EXECUTE messages.This PR is part of Native Protocol v5 support.