Shard awareness extension for Scylla #1206

dahankzter · 2018-10-05T06:48:15Z

This PR brings shard awareness to the token aware policy for Scylla servers version 2.3 or later.

This is done by implementing the Scylla protocol extension mechanism as as defined by:
https://github.com/scylladb/scylla/blob/master/docs/protocol-extensions.md
The impact for Scylla users is significant improvement in performance while still maintaining the original performance for Cassandra users.
Both latency and throughput are greatly improved but especially latency can see an order of magnitude improvement.

No config is needed for the user to benefit from this patch, the driver will discover the sharding support on its own.

JvGinkel · 2018-10-05T19:21:24Z

conn_test.go

@@ -828,14 +828,19 @@ func TestFrameHeaderObserver(t *testing.T) {

 	frames := observer.getFrames()

-	if len(frames) != 2 {
+	if len(frames) != 3 {
 		t.Fatalf("Expected to receive 2 frames, instead received %d", len(frames))


Do you need to adjust this message now also from 2 frames into 3 frames?

The protocol specifies an additional options query to discover the sharding info which cannot be statically read once since it's a runtime connection property. See the Conn::startup method. Afaik the test then needs to be accommodated. Is this the wrong way to verify this? If there is a better please let me know I am open to suggestions.

Ouch @JvGinkel I read your message too loosely it seems.
I will correct the message. Good catch!

alourie

So, there are a few nits with the code itself, but I have some reservations about merging all of it in the current form. The issue is clearly with the fact that it's an addition to include a different SERVER project's properties. I'm definitely not against supporting multiple endpoints, but something in the way it is organised now rubs me the wrong way.

I would probably ask for 2 separate PRs: one for the refactored connPicker implementation, and another for scylla support. But I would like to see the Scylla support as clearly separated, like maybe using "backends" module, so that main thing is used as a common base (which is, clearly, C*), and then additional backends (like Scylla) do their additional/different implementations.

CC @Zariel

alourie · 2018-10-08T02:00:35Z

connpicker.go

+type DefaultConnPicker struct {
+	conns []*Conn
+	pos   uint32
+	size  int


I'd use an unsigned int variant for the size.

It's very viral. This property is threaded through the code and it makes sense to have them as uint but perhaps not in this patch?

alourie · 2018-10-08T02:01:23Z

connpicker.go

+}
+
+func NewDefaultConnPicker(size int) *DefaultConnPicker {
+	return &DefaultConnPicker{


check for size to be non-zero to avoid errors?

I can check it here and return an error. If so what do we do then, panic or default to something?

There is no real infrastructure available to handle errors in this. It all relies on the config defaulting to 2. We can change the assumptions but I'd rather not in a patch that is about something else.

What do you think?

I think panic here is fine.

alourie · 2018-10-08T02:03:11Z

connpicker.go

+
+	for i, candidate := range p.conns {
+		if candidate == conn {
+			p.conns[i] = conn


p.conns[i] == nil?

p.conns[i] = nil perhaps? I think we should maybe eve resize it what do you think?

I have set it to nil now. We can change it if you prefer but the rest of the code works guards against nil.

nah, nil is fine. That's exactly what I meant :-)

alourie · 2018-10-08T02:12:28Z

connpicker.go

+}
+
+func (p *DefaultConnPicker) Pick(token) *Conn {
+	pos := int(atomic.AddUint32(&p.pos, 1) - 1)


Not sure the initial reason for this, but would you mind just reading atomically directly?

This keeps track of the current position in the conns list. I think it was always like this to step to the next connection in one atomic go.

alourie · 2018-10-08T02:43:17Z

policies.go

+}
+
+func (host selectedHost) Info() *HostInfo {
+	return (*HostInfo)(host.info)


probably no need to cast here

dahankzter · 2018-10-08T05:44:57Z

About your major comment @alourie I would also want to isolate and modularize it more but when we implemented this we saw now real option without major refactoring which felt very invasive for such a relatively small patch. It depends of course on what you mean with this backends module? A factory struct that knows how to instantiate backends?

I will try to address your minor issues asap, thanks for noticing. Nitpicks accumulate so it's good to catch them early!

alourie · 2018-10-08T13:48:14Z

@dahankzter I don't know what's the best way to implement backends, for now, would need to wait until @Zariel has an opinion. Maybe in the next major API change.

But splitting this into 2 PRs should not be complicated, no? The connection picker and the Scylla's implementation are quite separate, so I'd assume it would be possible to do.

dahankzter · 2018-10-08T14:28:17Z

We can probably split them up but the assignment of the picker to use is dynamic so at some point the code will blend.

There are basically 2 reasons for the code being as is today.

Make as small an impact as possible on the core code.
Make the usage for users as small as possible. They only need to update the driver now for it to kick in.

That's it really we can make any sort of bootstrapping otherwise if we don't want 2.
Maybe there even is a way to implement it more isolated that we haven't thought of and while we prefer no-config for end users we are not married to the idea. We want to be pragmatic about it and a config option can work fine as well. We just need to tell users to enable it.

mmatczuk · 2018-10-09T20:40:00Z

Hi @alourie,
I have fixed the code to limit scope of Scylla related stuff as much possible. It's split into multiple commits and two PRs. Please find the gocql generic changes in #1210 and Scylla specific changes in #1211 (note the dependency).

cc @dahankzter

This is an extension to tokenAwareHostPolicy supported by the Scylla 2.3 and onwards. It allows the driver to select a connection to shard on a host based on token. The protocol extension spec is available at [1]. [1] https://github.com/scylladb/scylla/blob/master/docs/protocol-extensions.md Co-authored-by: Michał Matczuk <[email protected]>

dahankzter · 2018-10-10T10:32:24Z

Lets work off of #1210 and #1211 instead.

alourie · 2018-10-10T13:18:56Z

👍

JvGinkel reviewed Oct 5, 2018

View reviewed changes

alourie suggested changes Oct 8, 2018

View reviewed changes

This was referenced Oct 9, 2018

Shard awareness extension for Scylla patchset 1 #1210

Open

Shard awareness extension for Scylla patchset 2 #1211

Open

dahankzter and others added 3 commits October 10, 2018 09:24

Fixed a test race in TestTracing.

e269777

increased timeout headroom in query timeout test

6c2f12e

dahankzter closed this Oct 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shard awareness extension for Scylla #1206

Shard awareness extension for Scylla #1206

dahankzter commented Oct 5, 2018

JvGinkel Oct 5, 2018

dahankzter Oct 6, 2018

dahankzter Oct 7, 2018

alourie left a comment

alourie Oct 8, 2018 •

edited

Loading

dahankzter Oct 8, 2018

alourie Oct 8, 2018

dahankzter Oct 8, 2018

dahankzter Oct 8, 2018

alourie Oct 8, 2018

alourie Oct 8, 2018

dahankzter Oct 8, 2018

dahankzter Oct 8, 2018

alourie Oct 8, 2018

alourie Oct 8, 2018

dahankzter Oct 8, 2018

alourie Oct 8, 2018

alourie Oct 8, 2018

dahankzter Oct 8, 2018

dahankzter commented Oct 8, 2018

alourie commented Oct 8, 2018

dahankzter commented Oct 8, 2018

mmatczuk commented Oct 9, 2018

dahankzter commented Oct 10, 2018

alourie commented Oct 10, 2018

Shard awareness extension for Scylla #1206

Shard awareness extension for Scylla #1206

Conversation

dahankzter commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alourie left a comment

Choose a reason for hiding this comment

alourie Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahankzter commented Oct 8, 2018

alourie commented Oct 8, 2018

dahankzter commented Oct 8, 2018

mmatczuk commented Oct 9, 2018

dahankzter commented Oct 10, 2018

alourie commented Oct 10, 2018

alourie Oct 8, 2018 •

edited

Loading