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

Extends gocql API for scylla shard aware info #164

Closed
wants to merge 2 commits into from

Conversation

moguchev
Copy link

@moguchev moguchev commented Apr 8, 2024

In our case, we encountered a problem when we need to insert and read more than 500 different keys (from different partitions) at a time in scyllaDB in one user request.

The insertion (BATCH request) still worked well, but reading queries (SELECT WHERE IN) left much to be desired.

We needed to extend the gocql API to be able to split keys/queries across hosts (or shards) on the side of our application and send queries to scyllaDB. This allowed us to reduce response time.

It might be useful to give this functionality. Well, or you can suggest a more elegant implementation than what I wrote :)

@moguchev moguchev force-pushed the get_shard_aware_info branch from ebcdfc0 to 2376551 Compare April 8, 2024 10:37
@sylwiaszunejko
Copy link
Collaborator

@moguchev Please squash your commits into set of independent changes. Correction commit affecting files changed in other PR commit should be squashed.

@moguchev moguchev force-pushed the get_shard_aware_info branch from 13a7697 to 2a9aa74 Compare April 15, 2024 10:00
@moguchev
Copy link
Author

@moguchev Please squash your commits into set of independent changes. Correction commit affecting files changed in other PR commit should be squashed.

done

sylwiaszunejko
sylwiaszunejko previously approved these changes Apr 25, 2024
@sylwiaszunejko
Copy link
Collaborator

@avelanarius Could you please review this PR to ensure everything is correct?

@moguchev moguchev force-pushed the get_shard_aware_info branch from 2a9aa74 to 2d81d51 Compare April 27, 2024 08:58
@moguchev
Copy link
Author

I've changed go:build tags in unit tests and in integration tests.
Also I don't know why ./integration.sh tablet fails. My function should not affect on it.

@moguchev
Copy link
Author

Also it's better to use ShardAwareRoutingInfo with enhancement from #165 because HostName will have a lot of calls in app.

@sylwiaszunejko
Copy link
Collaborator

@avelanarius Could you please review this PR to ensure everything is correct?

@avelanarius ping

@sylwiaszunejko
Copy link
Collaborator

@dkropachev Could you look at this PR?

@dkropachev
Copy link
Collaborator

dkropachev commented Jun 19, 2024

@moguchev , thanks for contribution, I see you put lots of effort into it.

@sylwiaszunejko, I think we should not merge it as it is, functionally it is correct, except not covering tablets feature.
But there are major concerns:

  1. It breaks single responsibility principle in regarding of what holds/generates routing information, currently it is tokenAwareHostPolicy but this PR makes Session do it as well.
  2. Which leads to bad maintainability, for example if tablet feature logic have to be replicated in GetShardAwareRoutingInfo
  3. This code is also does lot's of assumptions regarding configuration, for instance partitioner, HostSelectionPolicy and ConnPicker

Overall case is very valueable, not only for this particular driver.
Looking at the code, i beileve that we should extract and expose code that calculates routing information as it is done here, but on HostSelectionPolicy level and have an API for it on Session level, also to provide nice API to solve problem that is outlined in comments of GetShardAwareRoutingInfo, this API probably to be exposed on Session level as well

Update

Just FYI: rust-driver related issues: scylladb/scylla-rust-driver#468, scylladb/scylla-rust-driver#975, scylladb/scylla-rust-driver#944

@moguchev moguchev closed this Jun 28, 2024
@moguchev
Copy link
Author

Thanks! I hope you'll add this in API

@dkropachev
Copy link
Collaborator

Thanks! I hope you'll add this in API

Thanks for you contribution, it give us very good clue, I have created an issue based on this PR, we would be happy to see your recommendations/comments there.

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.

5 participants