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.

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

Cassandra flags #853

Merged
merged 9 commits into from
Oct 21, 2022
Merged

Cassandra flags #853

merged 9 commits into from
Oct 21, 2022

Conversation

conorbros
Copy link
Member

@conorbros conorbros commented Oct 12, 2022

Pre-reqs:

At the moment Shotover is just stripping flags from messages. This PR implements support for flags. Currently this PR just tests we can send flags without error, but the followup: #854 uses this support to get tracing information from queries sent through Shotover.

  • switch to shotover fork of cassandra-cpp

@rukai
Copy link
Member

rukai commented Oct 12, 2022

Why do we need to carry the flags around?
Shouldnt the flag automatically get set when tracing_id is Some?

@conorbros
Copy link
Member Author

Because tracing_id is None for requests, but the tracing flag is set so Cassandra attaches a tracing_id to the response.

@rukai
Copy link
Member

rukai commented Oct 12, 2022

Oooh, I see, thanks!
I'll do some research

@conorbros
Copy link
Member Author

@rukai
Copy link
Member

rukai commented Oct 12, 2022

A critical goal for shotover's message format is to make it so that transform developers can modify or read fields of the message with the expectation that there is a single source of truth. And I think this design could be improved to meet that.

Sure, a transform can change the tracing_id and have the flag automatically set to the correct value at encode time.
But for transforms further down the chain they will hit an inconsistency, they might for example check the value of the tracing flag and not realize that the tracing_id is out of sync.

I can think of two alternative approaches.
Note that I'm not making any argument how cassandra-protocol should represent this, that can be decided separately.

Alternative 1

enum Tracing {
    Request { request_tracing_id: bool }
    Response (Uuid)
}

struct CassandraFrame {
    tracing: Tracing,
    ...
}

This does introduce a new way to represent an invalid state: Tracing::Request with a response CassandraOperation.
But the existing implementation could already hit the same problem already by having tracing_id: Some with a request CassandraOperation.

Alternative 2

pub enum CassandraOperationRequest {
    Query {
        query: Box<CassandraStatement>,
        params: Box<QueryParams>,
    },
    Execute(Box<BodyReqExecuteOwned>),
    Register(BodyReqRegister),
    Batch(CassandraBatch),
    ...
}

pub enum CassandraOperationResponse {
    Result(CassandraResult),
    Error(ErrorBody),
    Prepare(Vec<u8>),
    Event(ServerEvent),
    ...
}

pub enum Direction {
    Request { operation: CassandraOperationRequest, tracing: bool },
    Response { operation: CassandraOperationResponse, tracing: Uuid },
}

struct CassandraFrame {
    direction: Direction,
    ...
}

This completely eliminates invalid states, but it is pretty invasive so we could leave it as a possible follow up refactor if you want.

@conorbros conorbros marked this pull request as ready for review October 13, 2022 11:31
@conorbros
Copy link
Member Author

I decided to go with option 1 since it's much simpler to implement and doesn't block this PR anymore. I'll add note to add considering option 2 for a followup.

@conorbros
Copy link
Member Author

conorbros commented Oct 19, 2022

In the interest of moving this pre-req PR forward since it's blocking token aware routing, I think we should carry on with reviews and I'll open a followup PR with the above changes mentioned.

Technically we are not regressing since it was always possible to modify the tracing_id, and it would've always been broken because the flags weren't correct.

@rukai
Copy link
Member

rukai commented Oct 19, 2022

I've made a prereq PR to unblock this while avoiding adding the flags field. #869
Once its landed you can just set tracing_id to Some(Uuid::nil()) on the request and then at encode time Flags::TRACING will get set while the dummy tracing uuid is unused.

This API is not ideal, but its progressing in the right direction.

@rukai
Copy link
Member

rukai commented Oct 19, 2022

we will also need to make a shotover/cassandra-cpp fork in order to progress this PR, since we havent heard back about cassandra-rs/cassandra-rs#147

@github-actions
Copy link

1 benchmark regressed. 0 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/3294545039

cassandra/redis_cache_select_uncached
                        time:   [1.2570 ms 1.2956 ms 1.3390 ms]
                        thrpt:  [746.80  elem/s 771.84  elem/s 795.52  elem/s]
                 change:
                        time:   [+22.030% +28.899% +35.889%] (p = 0.00 < 0.05)
                        thrpt:  [-26.410% -22.420% -18.053%]
                        Performance has regressed.

@rukai rukai merged commit d9c0a43 into shotover:main Oct 21, 2022
@conorbros conorbros deleted the tracing branch October 21, 2022 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants