Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

scylla-proxy: Add option to filter out control connection messages #863

Merged
merged 4 commits into from
Nov 21, 2023
Merged

scylla-proxy: Add option to filter out control connection messages #863

merged 4 commits into from
Nov 21, 2023

Conversation

Lorak-mmk
Copy link
Collaborator

@Lorak-mmk Lorak-mmk commented Nov 20, 2023 •
edited
Loading

Proxy does not allow filtering of control connection messages. Right now this is not a problem, as all queries performed by driver are not prepared, so tests can filter them out by checking query content.

The ongoing serialization refactor changes that and prepares some queries performed automatically by driver, which breaks some tests, as they are not able to differentiate test's queries from internal driver queries.

This PR adds a mechanism to filter out control connection messages by ignoring messages on connections that had any event registered. Control connection is the only one in rust driver that does it, so this should by a reliable heuristic.

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.

@Lorak-mmk Lorak-mmk changed the title Proxy filter controll conn scylla-proxy: Add option to filter out control connection messages Nov 20, 2023
@Lorak-mmk Lorak-mmk requested a review from piodul November 20, 2023 12:15
@Lorak-mmk
Copy link
Collaborator Author

I'll try to add some test of this feature

@Lorak-mmk Lorak-mmk force-pushed the proxy-filter-controll-conn branch from f18588b to edce2a4 Compare November 20, 2023 15:10
@Lorak-mmk
Copy link
Collaborator Author

I added a test.
I also added tokio's signal feature to dev dependencies in scylla-proxy - without it it's not possible to run cargo test inside scylla-proxy directory. I hope it's small enough change to not require another PR.

@Lorak-mmk
Proxy: Allow filtering out control connection messages
4e0d540 
There is no way to filter out Execute messages like we can with Query
messages, so if there are some sent by the driver itself (which will
start happening soon) we can't reliably perform tests using proxy.

This commit implements a heuristic way to filter out control connection
messages. The idea is that only control connection registers to CQL
Events, so we can skip connections that issued any registration message.
@Lorak-mmk
integration/consistency.rs: ignore control connection messages
35f7717
@Lorak-mmk
integration/shards.rs: ignore control connection messages
3d58669
@Lorak-mmk Lorak-mmk force-pushed the proxy-filter-controll-conn branch from edce2a4 to d3313e2 Compare November 20, 2023 15:18
@Lorak-mmk Lorak-mmk mentioned this pull request Nov 20, 2023
Merged
12 tasks
piodul
piodul reviewed Nov 21, 2023
View reviewed changes
scylla-proxy/Cargo.toml
@@ -31,3 +31,4 @@ rand = "0.8.5"
assert_matches = "1.5.0"
ntest = "0.9.0"
tracing-subscriber = { version = "0.3.14", features = ["env-filter"] }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit adds nothing useful to scylla-proxy, only an empty line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the reason why it used to work: I suppose that Cargo resolves dependencies for all crates in the workspace at once, and some other crate than scylla-proxy depends on tokio and enables the "signal" feature.

@piodul
Copy link
Collaborator

piodul commented Nov 21, 2023 

failures:

---- transport::connection::tests::test_coalescing stdout ----
Unique name: test_rust_1700493980_11
thread 'transport::connection::tests::test_coalescing' panicked at scylla/src/transport/connection.rs:2034:74:
called `Result::unwrap()` on an `Err` value: DbError(Unprepared { statement_id: b"i\xb60\x01/\xdc\x02\xb6\xec;\x18\x8e\xaa\xf38\xcf" }, "Prepared query with ID 69b630012fdc02b6ec3b188eaaf338cf not found (either the query was not prepared on this host (maybe the host has been restarted?) or you have prepared too many queries and it has been evicted from the internal cache)")

Strange; I know that we now sometimes prepare statements in case of query as well, but it falls back to execute which automatically reprepares the statement (but just once).

@Lorak-mmk
Copy link
Collaborator Author

This failing test can't be result of my changes, can it? It doesn't use proxy and I didn't touch scylla crate apart from 2 other tests.

@Lorak-mmk
scylla-proxy: Add Tokio signal feature to dev deps
7092296 
Some examples in scylla-proxy use tokio::signal, but this feature is not
enabled in dependencies, and this prevents running cargo test in
scylla-proxy directory. For some reason cargo test works when run in
workspace, I'm not sure why that is the case.
@Lorak-mmk Lorak-mmk force-pushed the proxy-filter-controll-conn branch from d3313e2 to 7092296 Compare November 21, 2023 10:07
@piodul
Copy link
Collaborator

piodul commented Nov 21, 2023

This failing test can't be result of my changes, can it? It doesn't use proxy and I didn't touch scylla crate apart from 2 other tests.

I agree that it isn't related to the PR, no need to fix it here. Please create an issue about this failure.

@Lorak-mmk Lorak-mmk mentioned this pull request Nov 21, 2023
Open
@Lorak-mmk
Copy link
Collaborator Author

I agree that it isn't related to the PR, no need to fix it here. Please create an issue about this failure.

Opened: #864

@piodul piodul merged commit 156ee60 into scylladb:main Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piodul piodul piodul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Lorak-mmk @piodul