Shard awareness extension for Scylla patchset 1 #1210

mmatczuk · 2018-10-09T20:13:37Z

This patchset contains the generic part of #1206. It introduces ConnPicker interface that is used to abstract hostConnPool storage.

There are, however, some differences to #1206 code.

Conn.shard is replaced by Conn.supported, the supported map
HostInfo.shardingInfo is dropped
all the ConnPicker impl are not exported
hostConnPool.connPickerOnce is dropped as it's already handled by mu

alourie

please rebase on newest master, specifically query_executor.go needs an update.

alourie · 2018-10-10T05:18:37Z

connpicker.go

+}
+
+func (nopConnPicker) Size() (int, int) {
+	return 0, 1 // 1 makes hostConnPool want to fill the pool


is there a reason to want to fill the pool for no-op?

nopConnPicker is a guard implementation used in newHostConnPool [1]. If it's 0 then hostConnPool.fill is not called. The real ConnPool assignment is deferred to the moment when we have a connection [2]. Maybe we should change the comment, do you have any suggestions?

[1] https://github.com/gocql/gocql/pull/1210/files#diff-3aa88c8ac2c0ffb49cbd55573005b354R283
[2] https://github.com/gocql/gocql/pull/1210/files#diff-3aa88c8ac2c0ffb49cbd55573005b354R515

I'm having a couple of ideas here, will try something out first.

ok, I'm fine with this guard. Maybe update the comment that if hostConnPool doesn't need to fill the pool, then it won't initiate the picker in this case?

I added more detailed description, PTAL.

dahankzter · 2018-10-10T07:02:25Z

They are both rebased @alourie afaict. No way to retrigger Travis though, can you do it?

dahankzter · 2018-10-10T07:20:40Z

Actually, I was wrong. I have rebased them now.

mmatczuk · 2018-10-10T08:48:09Z

Hi @alourie,
after rebase to aa46e85 it fails on Travis with C* 3.11.3, seems to be a problem with the commit.

alourie · 2018-10-10T08:53:32Z

@mmatczuk yep, I can see that, that's unrelated to your commit. Will review it soon.

mmatczuk · 2018-10-11T06:54:26Z

@alourie it's rebased to 26b68fd now.

alourie

cc @Zariel please have a look

mmatczuk · 2018-10-12T10:43:15Z

@Zariel I rebased that over you latest commit and resolved conflicts in conn.go

mmatczuk · 2018-10-18T07:22:02Z

Hi @alourie and @Zariel,

I rebased this branch again and dropped ea1cb17 due to that being redone in #1217. We also found and fixed a race in tests (see 8bd5d33) do you want that in a separate PR?

@dahankzter updated #1211 with graphs showing why this work so much beneficial to Scylla users. Do you think we could get that merged?

alourie · 2018-10-18T11:55:25Z

@mmatczuk If you can split 8bd5d33 into a separate PR, that would be great. We'll be able to proceed with that one unrelated to Scylla work.

Aside for that, I'm keen to merge this work, just waiting for @Zariel to have a look.

mmatczuk · 2018-10-18T12:19:08Z

The new PR is created #1220.

alourie · 2018-10-18T12:34:53Z

The new PR is created #1220.

Merged.

Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

This allows tokenAwareHostPolicy return token from ExecutableQuery. Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

The hostConnPool logic around conns slice is moved to defaultConnPicker. Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

mmatczuk · 2018-10-18T13:57:34Z

Great. Rebased again, only relevant commits are left.

rkuska · 2018-11-13T12:29:50Z

Hi, just wanted to ask whether you plan to merge this MR and also the other related; #1211 and if there is some sort of timeline set or some help is needed?

This would help me in deciding whether or not use the patched version of gocql.

mmatczuk · 2019-02-20T11:30:39Z

Hi @Zariel,
I'm trying to understand the status of the Scylla related patches, what is your take on it? Should gocql be a pure Cassandra driver without any vendor specific extensions? I'm fine with that, we already have a fork but from Open Source philosophy standpoint I'm not too happy about it.
Cheers

steeve · 2019-10-20T21:20:29Z

Friendly ping :)

mmatczuk · 2019-10-21T08:42:56Z

@steeve we maintain a shard aware fork at https://github.com/scylladb/gocql

steeve · 2019-10-21T14:18:20Z

@mmatczuk yes, we already use it and it great. but it would be nice that the changes be merged, even though the fork is top notch

martin-sucha · 2020-02-05T11:05:46Z

@Zariel could you please decide whether the Scylla patch set from scylla/gocql would be accepted (and what changes might be necessary before that) to gocql/gocql or not? If I understand it is (or should be made possible to make it) compatible both with Scylla and Cassandra at the same time. The uncertainty is causing some additional work for us and fracturing the ecosystem. I believe everyone would benefit from all the parties contributing just to a single version of gocql.

ashtonian · 2020-12-19T00:54:05Z

Please merge :)

jeffwidman · 2021-03-10T19:14:28Z

@Zariel could you please decide whether the Scylla patch set from scylla/gocql would be accepted (and what changes might be necessary before that) to gocql/gocql or not? If I understand it is (or should be made possible to make it) compatible both with Scylla and Cassandra at the same time. The uncertainty is causing some additional work for us and fracturing the ecosystem. I believe everyone would benefit from all the parties contributing just to a single version of gocql.

@martin-sucha @Zariel any update on this?

I completely agree it's a royal pain for us to ensure all our libraries are pulling in scylla/gocql since many of our external deps pull in gocql/gocql and would be much simpler if gocql just pulled in the token-aware stuff.

Zariel · 2021-03-10T19:46:31Z

I'll take a look at this this coming weekend, I'll be happy to support an interface which allows the Scylla specific logic to live outside the main tree.

jeffwidman · 2021-03-15T17:23:10Z

I'll take a look at this this coming weekend, I'll be happy to support an interface which allows the Scylla specific logic to live outside the main tree.

@Zariel any chance you got time to look at this over the weekend?

Zariel · 2021-03-15T18:51:20Z

So I think this is broadly ok but I think the abstraction could do with some work. It feels like the ConnPicker is doing a bit too much, namely controlling how many connections to open. I get this is what Scylla needs but it is one of the core reasons why I disliked the previous approach, needing to dial a connection to figure out how many connections to dial means that abstractions leak and makes things elsewhere a bit messy.

We could also do with having a type for Pick which exposes more than just the queries token, or at least so we can add things to it in the future without making breaking API changes.

Also the name Pick is too overused and not very descriptive, it could also probably do with returning some kind of iterator to try more connections or something.

mmatczuk · 2021-03-16T08:42:49Z

There is more Scylla specific stuff in the fork at the moment. The shard-awareness seems to be well battle tested. I can send an updated patch set based on what we have now.

jeffwidman · 2021-04-01T18:48:10Z

nudge @mmatczuk

robd003 · 2021-12-08T01:21:12Z

Any chance this could be merged in along with #1211 ?

ashtonian · 2022-07-08T15:47:12Z

@mmatczuk @Zariel - this pr turns 4 in October. What needs to be done with this and #1211 ?

DanielHe4rt · 2024-10-16T14:05:06Z

6 years and it still here. What's missing to get merged?

dkropachev · 2024-10-16T14:33:19Z

Lot's of things changed on scylla side since this PR was created, we need to review it again.

This was referenced Oct 9, 2018

Shard awareness extension for Scylla patchset 2 #1211

Open

Shard awareness extension for Scylla #1206

Closed

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch 2 times, most recently from 7a1a249 to d14fa44 Compare October 9, 2018 20:59

alourie suggested changes Oct 10, 2018

View reviewed changes

dahankzter force-pushed the mmt/scylla_shard_aware_patch_1 branch from d14fa44 to 5a13421 Compare October 10, 2018 07:19

dahankzter force-pushed the mmt/scylla_shard_aware_patch_1 branch from 5a13421 to 15fb290 Compare October 10, 2018 07:27

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch from 15fb290 to 9b76938 Compare October 10, 2018 08:33

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch 2 times, most recently from 917951f to 191e22e Compare October 11, 2018 06:52

alourie requested a review from Zariel October 11, 2018 07:33

alourie approved these changes Oct 11, 2018

View reviewed changes

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch from 191e22e to 6fdb280 Compare October 12, 2018 10:39

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch from 6fdb280 to db738dc Compare October 18, 2018 07:11

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch from db738dc to fbe2d0d Compare October 18, 2018 07:52

mmatczuk and others added 3 commits October 18, 2018 15:41

conn: introduced supported field containing payload of SUPPORTED frame

2902264

Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

SelectedHost: added Token function

bc364b8

This allows tokenAwareHostPolicy return token from ExecutableQuery. Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

hostConnPool: introduced ConnPicker interface to abstract pool storage

608dba8

The hostConnPool logic around conns slice is moved to defaultConnPicker. Co-authored-by: Henrik Johansson <henrik@scylladb.com> Co-authored-by: Michał Matczuk <michal@scylladb.com>

mmatczuk force-pushed the mmt/scylla_shard_aware_patch_1 branch from fbe2d0d to 608dba8 Compare October 18, 2018 13:41

apache deleted a comment from senthilrag Oct 26, 2018

nivekuil referenced this pull request in nivekuil/seaweedfs Jun 28, 2021

Use scylla driver for cassandra

0a78a0d

robd003 mentioned this pull request Dec 18, 2021

Merge ScyllaDB patches #1589

Open

github-actions bot mentioned this pull request Apr 10, 2022

Link Checker Report scylladb/scylla-manager#3102

Closed

github-actions bot mentioned this pull request Apr 17, 2022

Link Checker Report scylladb/scylla-manager#3121

Closed

Shard awareness extension for Scylla patchset 1 #1210

Are you sure you want to change the base?

Shard awareness extension for Scylla patchset 1 #1210

Conversation

mmatczuk commented Oct 9, 2018 • edited Loading

alourie left a comment

Choose a reason for hiding this comment

alourie Oct 10, 2018

Choose a reason for hiding this comment

mmatczuk Oct 10, 2018

Choose a reason for hiding this comment

alourie Oct 10, 2018

Choose a reason for hiding this comment

alourie Oct 11, 2018

Choose a reason for hiding this comment

mmatczuk Oct 11, 2018

Choose a reason for hiding this comment

dahankzter commented Oct 10, 2018

dahankzter commented Oct 10, 2018

mmatczuk commented Oct 10, 2018 • edited Loading

alourie commented Oct 10, 2018

mmatczuk commented Oct 11, 2018

alourie left a comment

Choose a reason for hiding this comment

mmatczuk commented Oct 12, 2018

mmatczuk commented Oct 18, 2018

alourie commented Oct 18, 2018

mmatczuk commented Oct 18, 2018

alourie commented Oct 18, 2018

mmatczuk commented Oct 18, 2018 • edited Loading

rkuska commented Nov 13, 2018

mmatczuk commented Feb 20, 2019 • edited Loading

steeve commented Oct 20, 2019

mmatczuk commented Oct 21, 2019

steeve commented Oct 21, 2019

martin-sucha commented Feb 5, 2020

ashtonian commented Dec 19, 2020

jeffwidman commented Mar 10, 2021

Zariel commented Mar 10, 2021

jeffwidman commented Mar 15, 2021

Zariel commented Mar 15, 2021

mmatczuk commented Mar 16, 2021

jeffwidman commented Apr 1, 2021

robd003 commented Dec 8, 2021

ashtonian commented Jul 8, 2022

DanielHe4rt commented Oct 16, 2024

dkropachev commented Oct 16, 2024

mmatczuk commented Oct 9, 2018 •

edited

Loading

mmatczuk commented Oct 10, 2018 •

edited

Loading

mmatczuk commented Oct 18, 2018 •

edited

Loading

mmatczuk commented Feb 20, 2019 •

edited

Loading