Fix RoutingInfo consistency #743

wprzytula · 2023-06-12T16:54:29Z

A bug has been found that put wrong Consistency and SerialConsistency into RoutingInfo. In order to:

be sure that it's fully fixed,
prevent regressions,
check consistencies in other contexts,

the following is done:

proxy acquires deserialization capabilities for request frames; it understands Query, Execute and Batch frames,
a test is added to check that request frames contains consistencies that match the specified ones,
a test is added to check that RoutingInfo for LoadBalancingPolicy contains the specified consistencies,
as the latter test confirms the bug in Session, the affected methods are fixed,
minor refactor is done in Session to make fiddling with consistency clearer,
bonus: utilities for single-node dry-mode cluster are added, based on the bindings' test implementation. These will most probably prove useful for writing future tests.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
~~[ ] I have adjusted the documentation in ./docs/source/.~~
~~[ ] I added appropriate Fixes: annotations to PR description.~~

piodul

While the purpose of the PR is good, I think it is too bloated. There is a lot of code duplication in tests and proxy utilities, not to mention the last commit which doesn't really belong to this PR.

Request deserialization code should be put into scylla-cql, as it belongs there and not in scylla-proxy. At least, I don't think that we need to duplicate the structure definitions.
There is lots of code to be deduplicated or shortened in the tests that this PR adds,
The last commit needs to go out and be resent separately. I don't see how it belongs here.

Please try to reduce the number of lines added by this PR. I don't believe we need a nearly 1000-line PR to fix an issue about some parameters not being propagated properly in a handful of functions.

scylla-proxy/src/request.rs

scylla/tests/integration/consistency.rs

scylla/tests/integration/utils.rs

scylla-cql/src/frame/request/batch.rs

scylla-cql/src/frame/request/query.rs

scylla-cql/src/frame/request/batch.rs

scylla-cql/src/frame/request/query.rs

scylla-cql/src/frame/request/mod.rs

piodul · 2023-07-24T13:43:13Z

scylla/tests/integration/consistency.rs

+                    Condition::or(
+                        Condition::RequestOpcode(RequestOpcode::Execute),
+                        Condition::or(
+                            Condition::RequestOpcode(RequestOpcode::Batch),
+                            Condition::and(
+                                Condition::RequestOpcode(RequestOpcode::Query),
+                                Condition::BodyContainsCaseSensitive(Box::new(
+                                    *b"INTO consistency_tests",
+                                )),
+                            ),
+                        ),
+                    ),


It's so verbose...

Maybe we should consider implementing boolean operators for Conditions (not in this PR).

What do you mean by this? Short-circuiting operations, such as && or ||, are not implementable in terms of traits, due to traits being unable to force short-circuiting.

https://doc.rust-lang.org/std/ops/index.html

Note that the && and || operators are currently not supported for overloading. Due to their short circuiting nature, they require a different design from traits for other operators like BitAnd. Designs for them are under discussion.

Interesting, I didn't know that. Maybe we can consider using the & and | operators instead until they figure the logical ones out.

I'm in favour of explicit methods in case of combining Conditions. Idiomatic Rust doesn't switch to operator overloading, unlike C++.

I'm in favour of explicit methods in case of combining Conditions.

If by methods you mean non-static methods here, then I'd argue that they are similarly awkward as calling them without self.

Idiomatic Rust doesn't switch to operator overloading, unlike C++.

I don't think this is true. Operators are usually overloaded for mathematical objects such as bignums or vectors. In our case, you would construct a predicate, a boolean expression - boolean operator are a natural fit (well, we would need to use the bitwise equivalents but it would be close enough). I don't think it is a big abuse of the syntax.

Ultimately, I think we should just allow closure predicates that takes a context object which allows doing all those checks directly in the code (e.g. |ctx| match ctx.request_opcode() { ... }). This would lead to much more natural syntax and greater flexibility.

This is anyway out of this PR's scope. For now, let's stick to methods.

scylla/tests/integration/consistency.rs

scylla/src/transport/session.rs

This will be used in proxy, to inspect request frames from the driver.

The generated error contained wrong value list count: it failed to include the number of value lists that were already paired with statements. As a bonus, a cleaner `count()` implementation was added to `BatchValuesIterator` trait, instead of using a dirty workaround.

scylla-cql/src/frame/request/batch.rs

scylla/src/transport/connection.rs

scylla/src/transport/session.rs

scylla-cql/src/frame/request/mod.rs

The iterator is not an accurate choice: the Batch struct does not contain any iteration state, so a plain slice would be a better choice.

Before, only borrowed requests were allowed (for instance, Query contained the borrowed query contents as &str). For convenient deserialization and for test purposes, owned requests are preferred, so the definitions are made contain `Cow`s.

This will be used in some tests.

This is required for tests.

Introduces DeserializableRequest trait whose main goal is testing purposes.

One simply asserts that serialization composed with deserialization is identity. Another makes sure that unknown flags lead to frame rejection (as deserializing frame containing unknown flags will likely lead to misinterpretation of some unknown protocol extensions).

The mentioned trait essentially serves the purpose of serialization, so the change is justified. The name `Request` will be used for an umbrella enum containing various types of requests.

This enum provided unified interface over CQL requests, useful for testing. Now, it supports only Query, Execute and Batch (it panics if attempted to deserialize a different opcode) and provided getters for Consistency and SerialConsistency. In the future, it can be extended for further test checks.

One test checks for each [serial] possible consistency that the request frame indeed contains the specified [serial] consistency. Another test checks that RoutingInfo's consistencies match the specified ones. As the latter test confirmed the bug in Session regarding RoutingInfo, it is temporarily marked as ignored. The mark will be removed in the next commit, after the bug is fixed.

The test added in the previous commit proves that the routing info has wrong Consistency and SerialConsistency set. Therefore, Session::query_paged(), execute_paged() and batch() are all fixed to respect correct consistencies. The test's "ignore" mark is henceforth removed.

As access to the execution profile is anyway needed in the callers of `Session::run_query()` (in order to put correct Consistency and SerialConsistency into RoutingInfo), the acquired Arc<ExecutionProfileInner> is passed to `Session::run_query()` as another argument. This prevents duplication of Arc cloning.

There are three levels of consistency that are taken into account by the Session: default exec profile's, per-statement exec profile's and per-statement one. To avoid confusion, we should use naming that clearly distinguishes them. Therefore, ExecuteQueryContext's field is renamed from the vague `consistency` to `consistency_set_on_statement` (the one directly set), as it is exactly its semantics.

Apparently, routing info had wrong Consistency and SerialConsistency set in iterator, too.

wprzytula force-pushed the fix-routing-info-consistency branch from ea7a26f to 67b77e1 Compare June 12, 2023 19:42

wprzytula requested review from piodul and havaker June 12, 2023 19:42

wprzytula changed the title ~~Fix routing info consistency~~ Fix RoutingInfo consistency Jun 14, 2023

piodul added this to the 0.8.3 milestone Jun 16, 2023

piodul requested changes Jun 16, 2023

View reviewed changes

wprzytula force-pushed the fix-routing-info-consistency branch 2 times, most recently from 9c377a8 to d05bde9 Compare July 3, 2023 06:57

wprzytula requested a review from piodul July 3, 2023 06:58

wprzytula force-pushed the fix-routing-info-consistency branch from d05bde9 to 49e6aeb Compare July 3, 2023 07:04

wprzytula mentioned this pull request Jul 3, 2023

Shard aware batching - add Session::shard_for_statement & Batch::enforce_target_node #738

Open

8 tasks

wprzytula modified the milestones: 0.8.3, 0.9.0 Jul 5, 2023

piodul reviewed Jul 24, 2023

View reviewed changes

wprzytula added 2 commits July 27, 2023 12:58

scylla_cql: Impl TryFrom<u8> for BatchType

fde10bb

This will be used in proxy, to inspect request frames from the driver.

wprzytula force-pushed the fix-routing-info-consistency branch 3 times, most recently from 66c6f95 to 99458a2 Compare July 27, 2023 14:59

piodul reviewed Jul 27, 2023

View reviewed changes

scylla-cql/src/frame/request/batch.rs Outdated Show resolved Hide resolved

piodul reviewed Jul 27, 2023

View reviewed changes

scylla/src/transport/connection.rs Show resolved Hide resolved

wprzytula force-pushed the fix-routing-info-consistency branch from 99458a2 to 2efa6da Compare July 27, 2023 15:12

wprzytula requested a review from piodul July 27, 2023 15:13

piodul reviewed Jul 27, 2023

View reviewed changes

scylla/src/transport/session.rs Show resolved Hide resolved

piodul reviewed Jul 27, 2023

View reviewed changes

scylla-cql/src/frame/request/mod.rs Outdated Show resolved Hide resolved

scylla_cql: get rid of iterator in Batch

8b35084

The iterator is not an accurate choice: the Batch struct does not contain any iteration state, so a plain slice would be a better choice.

wprzytula force-pushed the fix-routing-info-consistency branch from e64228c to 6fc86b5 Compare July 27, 2023 16:24

wprzytula added 3 commits July 27, 2023 18:31

scylla_cql: derive test Debug for some requests

58ea66e

This will be used in some tests.

scylla_cql: derive test Eq for some types

234b2ca

This is required for tests.

wprzytula added 9 commits July 27, 2023 18:33

scylla_cql: add deser capabilities for requests

c3c2270

Introduces DeserializableRequest trait whose main goal is testing purposes.

scylla_cql: rename Request to SerializableRequest

92800d4

The mentioned trait essentially serves the purpose of serialization, so the change is justified. The name `Request` will be used for an umbrella enum containing various types of requests.

iterator: fix: set correct consistency in RoutingInfo

b1664a7

Apparently, routing info had wrong Consistency and SerialConsistency set in iterator, too.

wprzytula force-pushed the fix-routing-info-consistency branch from 6fc86b5 to b1664a7 Compare July 27, 2023 16:35

wprzytula requested a review from piodul July 27, 2023 16:42

piodul approved these changes Jul 27, 2023

View reviewed changes

piodul merged commit 7c2a3fd into scylladb:main Jul 27, 2023
8 checks passed

wprzytula deleted the fix-routing-info-consistency branch July 28, 2023 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RoutingInfo consistency #743

Fix RoutingInfo consistency #743

wprzytula commented Jun 12, 2023 •

edited

Loading

piodul left a comment

piodul Jul 24, 2023

wprzytula Jul 24, 2023

piodul Jul 24, 2023

wprzytula Jul 24, 2023

piodul Jul 24, 2023

wprzytula Jul 27, 2023

Fix RoutingInfo consistency #743

Fix RoutingInfo consistency #743

Conversation

wprzytula commented Jun 12, 2023 • edited Loading

Pre-review checklist

piodul left a comment

Choose a reason for hiding this comment

piodul Jul 24, 2023

Choose a reason for hiding this comment

wprzytula Jul 24, 2023

Choose a reason for hiding this comment

piodul Jul 24, 2023

Choose a reason for hiding this comment

wprzytula Jul 24, 2023

Choose a reason for hiding this comment

piodul Jul 24, 2023

Choose a reason for hiding this comment

wprzytula Jul 27, 2023

Choose a reason for hiding this comment

wprzytula commented Jun 12, 2023 •

edited

Loading