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

Shard awareness extension for Scylla patchset 2 #1211

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open

Shard awareness extension for Scylla patchset 2 #1211

wants to merge 7 commits into from

Conversation

mmatczuk
Copy link
Contributor

@mmatczuk mmatczuk commented Oct 9, 2018

This patchset contains the Scylla specific part of #1206, the relevant commit is b5f7b2e. This work depends on #1210. It's changed form #1206 to limit scope of Scylla related code.

This is an extension to tokenAwareHostPolicy supported by the Scylla 2.3
and onwards. It allows driver to select a connection to shard on a host
based on the token. The protocol extension spec is available at [1].

[1] https://github.com/scylladb/scylla/blob/master/docs/protocol-extensions.md

@mmatczuk mmatczuk mentioned this pull request Oct 9, 2018
Closed
@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from b5f7b2e to f25f8d4 Compare October 9, 2018 20:48
@dahankzter dahankzter force-pushed the mmt/scylla_shard_aware_patch_2 branch from f25f8d4 to 967e0b3 Compare October 10, 2018 07:29
alourie
alourie reviewed Oct 10, 2018
View reviewed changes
conn_test.go Outdated
}
readyFrame := frames[0]

supportedFrame := frames[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

supportedFrame is actually a gocql type, so rename to avoid confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, do you have an idea for a better name? We could also introduce var f ObservedFrameHeader and reassign that or even inline them all...

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, just call it something simpler, like sFrame, no need to overcomplicate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd fix that in the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I resolved to if f := frames[0]; f.Opcode != frameOp(opSupported) and applied that to other if branches because readyFrame is also a type.

@dahankzter
Copy link
Contributor

These are some results on scylla-2.3 using the applied patches vs master.
The left part of the graphs are with the patches applied and the right is in master.
The effect of the shard awareness very clear from the second image where the request served are displayed per shard.

reqs_served_per_node

reqs_server_per_shard

load_per_node

load_per_shard

@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from 967e0b3 to 06a786d Compare October 11, 2018 07:01
@dahankzter
Copy link
Contributor

The effect on latency is even greater as can be seen below. The left part of the graphs are master and the right with this patch applied.

latency_by_node_average

latency_by_node_95_percentile

latency_by_node_99_percentile

latency_by_shard_average

latency_by_shard_95_percentile

latency_by_shard_99_percentile

@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from 06a786d to 0e50e36 Compare October 17, 2018 17:01
@mmatczuk mmatczuk mentioned this pull request Oct 18, 2018
Open
@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from 0e50e36 to 7effd2a Compare October 18, 2018 14:00
@apache apache deleted a comment from senthilrag Oct 26, 2018
@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from 7effd2a to 1fe272b Compare January 31, 2019 13:24
@mmatczuk mmatczuk force-pushed the mmt/scylla_shard_aware_patch_2 branch from 1fe272b to f702adf Compare February 13, 2019 10:57
@dahankzter dahankzter force-pushed the mmt/scylla_shard_aware_patch_2 branch from f702adf to bd1a67a Compare March 6, 2019 08:46
mmatczuk and others added 7 commits March 26, 2019 10:45
@mmatczuk
conn: introduced supported field containing payload of SUPPORTED frame
afa7eab 
Co-authored-by: Henrik Johansson <[email protected]>
Co-authored-by: Michał Matczuk <[email protected]>
@mmatczuk
SelectedHost: added Token function
5963c95 
This allows tokenAwareHostPolicy return token from ExecutableQuery.

Co-authored-by: Henrik Johansson <[email protected]>
Co-authored-by: Michał Matczuk <[email protected]>
@mmatczuk
hostConnPool: introduced ConnPicker interface to abstract pool storage
10957f9 
The hostConnPool logic around conns slice is moved to defaultConnPicker.

Co-authored-by: Henrik Johansson <[email protected]>
Co-authored-by: Michał Matczuk <[email protected]>
@mmatczuk
scylla: dedicated token aware ConnPicker
51460b5 
This is an extension to tokenAwareHostPolicy supported by the Scylla 2.3
and onwards. It allows driver to select a connection to shard on a host
based on the token. The protocol extension spec is available at [1].

[1] https://github.com/scylladb/scylla/blob/master/docs/protocol-extensions.md

Co-authored-by: Henrik Johansson <[email protected]>
Co-authored-by: Michał Matczuk <[email protected]>
scylla: lazily close excess connections.
b794c9f 
A list of excess connections is maintained to allow for lazy removal of.
excess connections. Keeping excess connections open helps reaching equilibrium
faster since the likelihood of hitting the same shard decreases with the number
of connections to the shard. The excess connections list is currently
capped to 10 times the number of shards. This magic number has no
backing statistics but sounds reasonable.
scylla: give out random connections before pool equilibrium
f1af639 
When the pool is not yet fully materialized we give the caller
a random connection to allow the session to start working.
scylla: ensure we use the correct token to determine shard
1ad3b47 
The driver silently replaced the current token with the closest one
from the token ring that it could find. This worked well enough
to route the request to the correct host but to pick the right shard
we need the actual token for the qiven query.
@dahankzter dahankzter force-pushed the mmt/scylla_shard_aware_patch_2 branch from d172663 to 1ad3b47 Compare March 26, 2019 09:57
@dahankzter
Copy link
Contributor

@Zariel @alourie is there any way we could persuade you to accept this patch? We are updating it as we we find bugs but it isn't transparent to users where to go for the driver or where to contribute fixes.

@steeve
Copy link

steeve commented Oct 20, 2019

Friendly ping

@artem-v-shamsutdinov artem-v-shamsutdinov mentioned this pull request Dec 29, 2019
Open
@altanozlu
Copy link

Any news ?

@robd003 robd003 mentioned this pull request Dec 18, 2021
Open
@github-actions github-actions bot mentioned this pull request Apr 10, 2022
Closed
@github-actions github-actions bot mentioned this pull request Apr 17, 2022
Closed
@corporatepiyush
Copy link

Why is this being delayed ?

@DanielHe4rt
Copy link

Oh well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alourie alourie alourie left review comments

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

Successfully merging this pull request may close these issues.
7 participants
@mmatczuk @dahankzter @steeve @altanozlu @corporatepiyush @DanielHe4rt @alourie