transport: Implement SCYLLA_USE_METADATA_ID support#23292
transport: Implement SCYLLA_USE_METADATA_ID support#23292scylladb-promoter merged 3 commits intoscylladb:masterfrom
Conversation
andrzej-jackowski-scylladb
force-pushed
the
metadata-id-support
branch
from
e041368 to
1c445d4
Compare
andrzej-jackowski-scylladb
requested review from
ptrsmrn and
tgrabiec
as code owners
I believe we use 'Fixes:' - unsure if it matters much. |
Yes the automation relies on it. |
🔴 CI State: FAILURE✅ - Build Build Details:
|
@avikivity, I need a clarification. I can see so many commits that use "Closes" instead of "Fixes", for instance this one: a72dde2 I can start using "Fixes" instead "Closes" but currently I'm lost what is the proper, up to date rule how to use "Closes" and "Fixes". Especially, some of the commits use both (e.g. c082184) |
Look at the PR - #22462 - it 'fixes'. The merge commit closes the PR. |
That's a side effect of github inflexibility. We base pull request on master but commit to next, and github doesn't allow that from the UI. So we have a script scripts/pull_github_pr.sh to do that for us. It uses Closes to mark the pull request closed. So we have: Fixes - for marking an issue fixed, Closes - for marking a pull request closed. Having the separation prevents chasing the wrong number. |
andrzej-jackowski-scylladb
force-pushed
the
metadata-id-support
branch
from
1c445d4 to
075a1fb
Compare
andrzej-jackowski-scylladb
force-pushed
the
metadata-id-support
branch
from
6ab48f7 to
54d2a80
Compare
CQLv5 introduced metadata_id, which is a checksum computed from column names and types, to track schema changes in prepared statements. This commit introduces calculate_metadata_id to compute such id for given metadata. Please note that calculate_metadata_id() produces different hashes than Cassandra's computeResultMetadataId(). We use SHA256 truncated to 128 bits instead of MD5. There are also two smaller technical differences: calculate_metadata_id() doesn't add unneeded zeros and it adds a length of a string when an sstring is being fed to the hasher. The difference is intentional because MD5 has known vulnerabilities, moreover we don't want to introduce any dependency between our metadata_id and Cassandra's. This change: - Add cql_metadata_id_type - Implement metadata::calculate_metadata_id() - Add boost tests to confirm correctness of the function
Metadata id was introduced in CQLv5 to make metadata of prepared statement consistent between driver and database. This commit introduces a protocol extension that allows to use the same mechanism in CQLv4. This change: - Introduce SCYLLA_USE_METADATA_ID protocol extension for CQLv4 - Introduce METADATA_CHANGED flag in RESULT. The flag cames directly from CQLv5 binary protocol. In CQLv4, the bit was never used, so we assume it is safe to reuse it. - Implement handling of metadata_id and METADATA_CHANGED in RESULT rows - Implement returning metadata_id in RESULT prepared - Implement reading metadata_id from EXECUTE - Added description of SCYLLA_USE_METADATA_ID in documentation Metadata_id is wrapped in cql_metadata_id_wrapper because we need to distinguish the following situations: - Metadata_id is not supported by the protocol (e.g. CQLv4 without the extension is used) - Metadata_id is supported by the protocol but not set - e.g. PREPARE query is being handled: it doesn't contain metadata_id in the request but the reply (RESULT prepared) must contain metadata_id - Metadata_id is supported by the protocol and set, any number of bytes >= 0 is allowed, according to the CQLv5 protocol specification Fixes scylladb#20860
Implement corner-cases of prepared statement metadata, as described in scylladb#20860. Although the purpose of the test was to verify the newly implemented SCYLLA_USE_METADATA_ID protocol extension, the test also passes with scylla-driver 3.29.3 that doesn't implement the support for this extension. That is because the driver doesn't implement support for skip_metadata flag, so fresh metadata are included in every prepared statement response, regardless of the metadata_id. This change: - Add test_changed_prepared_statement_metadata_columns to verify a scenario when a number of columns changes in a table used by a prepared statement - Add test_changed_prepared_statement_metadata_types to verify a scenario when a type of a column changes in a table used by a prepared statement - Add test_changed_prepared_statement_metadata_udt to veriy a scenario when a UDT changes in a table used by a prepared statement I tested the code with a modified Python driver (ref. scylladb/python-driver#457): - If SKIP_METADATA is enabled (scylladb/python-driver@c1809c1) but not other changes are introduced, all three test cases fail. - If SKIP_METADATA is disabled (no scylladb/python-driver@c1809c1) all test cases pass because fresh metadata are included in each reply. - If SKIP_METADATA is enabled (scylladb/python-driver@c1809c1) and SCYLLA_USE_METADATA_ID extension is included (scylladb/python-driver@8aba164) all test cases pass and verifies the correctness the implementation.
andrzej-jackowski-scylladb
force-pushed
the
metadata-id-support
branch
from
54d2a80 to
8b660f0
Compare
|
v13:
v14:
|
🟢 CI State: SUCCESS✅ - Framework test Build Details:
|
andrzej-jackowski-scylladb
added
the
status/merge_candidate
|
@scylladb/scylla-maint please consider merging |
|
@scylladb/scylla-maint all recent Dmitry's comments are resolved. Please consider merging. |
10 participants
Metadata id was introduced in CQLv5 to make metadata of prepared
statement metadata consistent between driver and database.
This commit introduces a protocol extension that allows to use the same
mechanism in CQLv4. As CQLv5 is currently unsupported in ScyllaDb (as well
as in some of the drivers), the motivation is to allow fixing #20860.
This change:
- Implement metadata::calculate_metadata_id()
- Implement SCYLLA_USE_METADATA_ID protocol extension for CQLv4
- Added description of SCYLLA_USE_METADATA_ID in documentation
- Add boost tests to confirm correctness of the function
- Add python tests for table metadata change corner-cases
Fixes #20860
Also see related https://scylladb.atlassian.net/wiki/spaces/RND/pages/42238631/MetadataId+extension+in+CQLv4+Requirement+Document
No backport needed (unless specifically requested by a customer), because there are existing workarounds for the issue