Add QueryTypeFilter compatibility to CassandraSinkCluster#1995
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an incompatibility between QueryTypeFilter (which replaces filtered requests with Frame::Dummy) and CassandraSinkCluster (which previously assumed all requests had a Cassandra stream_id). It also introduces protocol-agnostic error classification so filtered Cassandra responses map to ErrorType::Invalid (avoiding defunct connections in the Datastax C++ driver), and adds an integration test reproducing the original failure.
Changes:
- Add
MessageErrorTypeand thread it through error-response construction so transforms can classify errors asInternalvsRejected(Cassandra maps toServervsInvalid). - Update
CassandraSinkCluster/ connection tracking to tolerate dummy messages instead of panicking on missingstream_id. - Add a Cassandra cluster integration test and config that places
QueryTypeFilterin front ofCassandraSinkCluster.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
shotover/src/transforms/tee.rs |
Updates Tee’s mismatch error construction to pass MessageErrorType. |
shotover/src/transforms/null.rs |
Updates NullSink error construction to pass MessageErrorType. |
shotover/src/transforms/filter.rs |
Updates QueryTypeFilter to classify filtered requests as Rejected. |
shotover/src/transforms/cassandra/sink_cluster/rewrite.rs |
Updates internal Cassandra rewrite error construction to pass MessageErrorType. |
shotover/src/transforms/cassandra/sink_cluster/mod.rs |
Avoids version detection using dummy frames; generates dummy responses for dummy requests during routing. |
shotover/src/transforms/cassandra/sink_cluster/connection.rs |
Stops unwrapping stream_id for requests; skips missing stream_ids. |
shotover/src/source_task.rs |
Updates internal “bug” error responses to use MessageErrorType::Internal. |
shotover/src/message/mod.rs |
Introduces MessageErrorType and adds it to to_error_response / from_*_to_error_response APIs with protocol mappings. |
shotover/src/frame/cassandra.rs |
Allows Cassandra error responses to specify ErrorType instead of hardcoding Server. |
shotover-proxy/tests/test-configs/cassandra/cluster-v4/topology-query-type-filter.yaml |
New test config chaining QueryTypeFilter -> CassandraSinkCluster. |
shotover-proxy/tests/cassandra_int_tests/mod.rs |
New integration test validating filtered writes return ErrorType::Invalid and subsequent queries still succeed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR first adds a new integration test to reproduce the incompatibility problem between
QueryTypeFilterandCassandraSinkCluster. This new test places aQueryTypeFilterin front ofCassandraSinkClusterin the transform chain which causes the error below (ref: example failed CI test). The problem here is thatQueryTypeFilterreplaces the filtered request with aFrame::Dummywhich doesn't have astream_id, and passes the dummy frame to the downstreamCassandraSinkClusterwhich then runs x.stream_id().unwrap(), leading to the error of callingOption::unwrap()on theNonevalue.To resolve this incompatibility issue, extra logic for handling dummy message is added to [shotover/src/transforms/cassandra/sink_cluster/connection.rs and shotover/src/transforms/cassandra/sink_cluster/mod.rs, allowing the new integration test to pass.
Additionally, the Cassandra error type for filtered responses is changed from
ErrorType::ServertoErrorType::Invalidbecause the Datastax C++ driver defuncts the connection on Server errors which breaks subsequent queries (other drivers handle server errors more gracefully and are fine).Specifically, the C++ driver produced this warning (
WARN datastax::internal::core::RequestExecution::on_error_response: Received server error 'Message was filtered out by shotover' from host 127.0.0.1. Defuncting the connection...) in the integration test, which then led to the errorthread 'cassandra_int_tests::cluster_single_rack_v4_query_type_filter::case_1_cpp' (5539) panicked at test-helpers/src/connection/cassandra/connection/cpp.rs:164:25: Unexpected cassandra_cpp error: Cassandra error LIB_NO_HOSTS_AVAILABLE: All hosts in current policy attempted and were either unavailable or failedbecause the driver regarded nodes as down after defuncting the connection. Example CI run here.A filtered query is a rejection instead of a server bug and so
Invalidis more appropriate thanServererror here. A new protocol-agonisticMessageErrorTypeenum is hence introduced to support passing the error type from the commonQueryTypeFilterto the protocol-specific response mapper.