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

CassandraSinkCluster rewrite system.local #744

Merged
merged 6 commits into from
Aug 23, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Aug 12, 2022

Misc changes:

  • The v4 uuid feature is needed to enable random uuid generation
  • The changes to ResultValue::new are needed to allow handling null values in cases that I hit where it wasnt. There are still other null value cases that its not handling properly but I dont know where cassandra will and wont return null values so I'm just leaving the rest to be dealt with when we hit them.
  • I introduced an Any variant to ResultValue, the idea here is to allow assert_query_results in cases where there are values that are non deterministic. assert_query_results provides an intuitive way to assert on both the length and the contents of the results so I think Any is a lot better approach then just manually doing something like assert_eq!(results[3], ResultValue::Varchar("blah"))
  • I had to stub out the system.peers table as the cassandra_cpp driver kept routing system.local queries directly to cassandra nodes instead of going through shotover causing my assertions on the system.local table results to fail.

system.local fields:

  • release_version - I take the lowest value from any node as this field is most likely to be used for the existence of features so if some nodes dont have a feature then we should consider shotover as not having that feature
  • tokens - combine all tokens from nodes in our dc-rack into a single token list, token generation is non-deterministic so we cant hardcode this.
  • schema_version - if any peer nodes or the local node (routed to init_handshake_connection) disagree on the schema version a random uuid is returned, otherwise the agreed schema version is returned. DDL queries are also routed to init_handshake_connection which means that a schema disagreement will always be conveyed immediately after a DDL query and it will last until the local node has received gossip that schema agreement has occurred.
  • listen_address - extract shotovers address from the client<->shotover socket, this gives us a sane default and the user may still rewrite this address via the CassandraPeersRewrite transform if needed.
  • broadcast_address - same as listen_address
  • host_id - configured host id. cassandra nodes will generate their own host id and store it without configuration. I didnt take that approach however because we want shotover to be completely stateless. And I think asking the user to manually set a host_id is better than introducing statefulness. Another option is to just randomly generate the host_id each time shotover starts up, but I think its better to be persistent in case drivers rely on that.
  • rack - configured rack
  • data_center - configured data center
  • everything else is just left as is from the init_handshake_connection's response

@rukai rukai force-pushed the rewrite_system_local branch 7 times, most recently from b8ecac9 to 80e25f7 Compare August 19, 2022 00:56
@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/2886374414

  5 (5.00%) high severe
redis/cluster_set       time:   [184.19 us 187.98 us 192.20 us]
                        thrpt:  [5.2028 Kelem/s 5.3197 Kelem/s 5.4291 Kelem/s]
                 change:
                        time:   [+22.312% +25.284% +28.340%] (p = 0.00 < 0.05)
                        thrpt:  [-22.082% -20.181% -18.242%]
                        Performance has regressed.

@rukai rukai force-pushed the rewrite_system_local branch 5 times, most recently from 641ed3a to 4912fc6 Compare August 22, 2022 02:05
@shotover shotover deleted a comment from github-actions bot Aug 22, 2022
@shotover shotover deleted a comment from github-actions bot Aug 22, 2022
@rukai rukai marked this pull request as ready for review August 23, 2022 01:50
@benbromhead
Copy link
Member

Nice!

@rukai rukai merged commit c5b4d6e into shotover:main Aug 23, 2022
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