Introduce support for tablets #249

sylwiaszunejko · 2023-08-02T07:17:06Z

This PR introduces changes to the driver that are necessary for shard-awareness and token-awareness to work effectively with the tablets.

Now if driver send the request to the wrong node/shard it will get the correct tablet information from Scylla in custom_payload. It uses this information to obtain target replicas and shard numbers for tables managed by tablet replication.

I also added parsing TABLETS_ROUTING_V1 extension to ProtocolFeatures. In order for Scylla to send the tablet info, the driver must tell the database during connection handshake that it is able to interpret it. This negotiation is added as a part of ProtocolFeatures class.

To test this change I created integration and unit tests.

Fixes: #281

cassandra/cluster.py

tests/integration/standard/test_tablets.py

tests/integration/__init__.py

cassandra/cluster.py

cassandra/query.py

cassandra/pool.py

cassandra/cluster.py

cassandra/metadata.py

Lorak-mmk

It looks much better now

cassandra/metadata.py

cassandra/policies.py

cassandra/pool.py

cassandra/cluster.py

cassandra/tablets.py

cassandra/cluster.py

cassandra/policies.py

cassandra/pool.py

cassandra/tablets.py

cassandra/policies.py

cassandra/cluster.py

cassandra/tablets.py

cassandra/cluster.py

cassandra/tablets.py

cassandra/pool.py

cassandra/cluster.py

cassandra/tablets.py

Lorak-mmk · 2023-08-10T13:41:08Z

oops, it was not supposed to be "Approve"

Lorak-mmk

Looks good now. Please add the comment that I requested and I think we can merge this.

sylwiaszunejko · 2023-08-11T10:35:14Z

Looks good now. Please add the comment that I requested and I think we can merge this.

In gocql @avelanarius said we should wait with merging until the events are available, I don't know if this also applies to python-driver

tgrabiec · 2023-08-14T12:07:22Z

cassandra/pool.py

+                tablet = self._session.cluster._load_balancing_policy._cluster_metadata._tablets.get_tablet_for_key(keyspace, table, t)
+
+                if tablet is not None:
+                    shard_id = tablet.replicas[0][1]


Why do you always take the shard of the first replica? This pool may be for a different replica.

But how to know which shard to take? Isn't it redundant? I thought the choice didn't matter so I took the first one, maybe I misunderstood something

Due to the current two-pass implementation (first we determine the node to send the request to, then we determine the shard to send the request to), I think this code has to go through all replicas, find the replica that matches the HostConnection's host and read its shard number.

@sylwiaszunejko IIUC this function chooses shard-connection to a particular host for a given key. The owning shard of a given key is different on different hosts. It's specified in the <host, shard> tuple in the replica list. So the shard should depend on which host-replica you're going to talk to.

Ok, now I get it, hope now it looks good

sylwiaszunejko · 2024-01-05T09:16:12Z

@Lorak-mmk @avelanarius I removed experimental setting

Lorak-mmk

Left some final minor nits, otherwise looks good to me

cassandra/cluster.py

cassandra/metadata.py

cassandra/protocol_features.py

In order for Scylla to send the tablet info, the driver must tell the database during connection handshake that it is able to interpret it. This negotation is added as a part of ProtocolFeatures class.

Lorak-mmk · 2024-01-08T11:35:26Z

cassandra/cluster.py

@@ -1762,6 +1764,8 @@ def connect(self, keyspace=None, wait_for_all_pools=False):
                try:
                    self.control_connection.connect()

+                    self.load_balancing_policy.populate(weakref.proxy(self), self.metadata.all_hosts())


Not sure if this code was here before most recent pushes, I don't remember seeing it.
Why do you need to pass weakref? Is the cluster saved somewhere?
Why do we need to call this here? I see that populate is called in other places already, so why another call here?

When we were using experimental setting, it was known from the beginning that we are using tablets. So, in every place where self.load_balancing_policy.populate() was called, it was properly informed to use tablets. But now, we have to get the information about using tablets from connection handshake. Therefore, when we are performing populate on lbp here: https://github.com/scylladb/python-driver/blob/master/cassandra/cluster.py#L1744, we do not know yet if tablets are being used or not, so there is a need to call populate again after calling connect() on control_connection.

Ok, but please put proper explanation in the comment, this code is really not obvious

@sylwiaszunejko It looks like this line specifically causes the CI failures. Since pretty much all integration tests don't use tablets (and your code shouldn't affect them), I tested progressively removing more and more of your changes and removing this line fixed the failing tests. I don't yet have an explanation why it is.

Branch on which I tested: https://github.com/avelanarius/python-driver/commits/tablets_disabled3/, "no populate" fixed the failing tests ("Build and upload to PyPi" are failing for some other reason)

avelanarius · 2024-01-09T10:00:23Z

I'll also look into test failures today and try to debug them.

avelanarius · 2024-01-09T16:40:25Z

Some initial investigation (still waiting for some runs to finish): master (without this PR) reliably passes, with PR rebased on current master fails. Now looking into what part of this PR causes the test failure.

sylwiaszunejko · 2024-01-12T09:18:00Z

@avelanarius I added documentation

avelanarius · 2024-01-12T10:07:46Z

cassandra/cluster.py

@@ -1775,6 +1777,9 @@ def connect(self, keyspace=None, wait_for_all_pools=False):
                    self.shutdown()
                    raise

+                # Update the information about tablet support after connection handshake.
+                self.load_balancing_policy._tablets_routing_v1 = self.control_connection._tablets_routing_v1


Isn't it necessary to set _tablets_routing_v1 also on child policies? This is what populate() also did. Maybe the code doesn't work properly if the set policy is some policy that has token aware policy as child - the child policy (token aware policy) won't have _tablets_routing_v1 set?

That's true, do you think that creating another function only to populate _tablets_routing_v1 will be a good solution? Or should I investigate more why calling populate was causing issues and return to calling it?

I think it's a good idea to investigate why populate was causing issues.

Add mechanism to parse system.tablets periodically. In TokenAwarePolicy check if keyspace uses tablets if so try to use them to find replicas. Make shard awareness work when using tablets. Everything is wrapped in experimental setting, because tablets are still experimental in ScyllaDB and changes in the tablets format are possible.

sylwiaszunejko requested a review from avelanarius August 2, 2023 08:37

avelanarius requested a review from Lorak-mmk August 2, 2023 08:39

Lorak-mmk requested changes Aug 3, 2023

View reviewed changes

sylwiaszunejko force-pushed the introduce_tablets branch 2 times, most recently from a436581 to 960cb95 Compare August 4, 2023 09:24

sylwiaszunejko requested a review from Lorak-mmk August 4, 2023 09:26

sylwiaszunejko force-pushed the introduce_tablets branch from 960cb95 to e1e572a Compare August 4, 2023 10:04

Lorak-mmk reviewed Aug 4, 2023

View reviewed changes

cassandra/cluster.py Outdated Show resolved Hide resolved

Lorak-mmk reviewed Aug 4, 2023

View reviewed changes

cassandra/metadata.py Outdated Show resolved Hide resolved

sylwiaszunejko force-pushed the introduce_tablets branch 5 times, most recently from 71a5c4f to ef837ab Compare August 7, 2023 13:07

sylwiaszunejko requested a review from Lorak-mmk August 7, 2023 13:15

sylwiaszunejko force-pushed the introduce_tablets branch from ef837ab to 94b8dc8 Compare August 9, 2023 06:51

Lorak-mmk requested changes Aug 10, 2023

View reviewed changes

sylwiaszunejko force-pushed the introduce_tablets branch from 94b8dc8 to 70e86d5 Compare August 10, 2023 10:35

sylwiaszunejko requested a review from Lorak-mmk August 10, 2023 10:41

Lorak-mmk approved these changes Aug 10, 2023

View reviewed changes

sylwiaszunejko force-pushed the introduce_tablets branch from 70e86d5 to 882a6f1 Compare August 11, 2023 06:22

sylwiaszunejko requested a review from Lorak-mmk August 11, 2023 06:25

sylwiaszunejko force-pushed the introduce_tablets branch from 882a6f1 to 2d7890f Compare August 11, 2023 06:26

Lorak-mmk approved these changes Aug 11, 2023

View reviewed changes

sylwiaszunejko force-pushed the introduce_tablets branch from 2d7890f to 55ea9bf Compare August 11, 2023 10:32

sylwiaszunejko requested a review from Lorak-mmk August 11, 2023 11:16

tgrabiec reviewed Aug 14, 2023

View reviewed changes

sylwiaszunejko force-pushed the introduce_tablets branch from 55ea9bf to 4ba3fc6 Compare August 14, 2023 18:28

sylwiaszunejko force-pushed the introduce_tablets branch 2 times, most recently from ff00514 to b0a571e Compare January 5, 2024 15:03

Lorak-mmk requested changes Jan 5, 2024

View reviewed changes

cassandra/cluster.py Outdated Show resolved Hide resolved

cassandra/metadata.py Outdated Show resolved Hide resolved

cassandra/protocol_features.py Outdated Show resolved Hide resolved

Add parsing TABLETS_ROUTING_V1 extension to ProtocolFeatures

e8d7151

In order for Scylla to send the tablet info, the driver must tell the database during connection handshake that it is able to interpret it. This negotation is added as a part of ProtocolFeatures class.

sylwiaszunejko force-pushed the introduce_tablets branch from b0a571e to c2ca988 Compare January 5, 2024 15:35

sylwiaszunejko requested a review from Lorak-mmk January 5, 2024 15:35

sylwiaszunejko force-pushed the introduce_tablets branch from c2ca988 to d160241 Compare January 8, 2024 08:58

Lorak-mmk reviewed Jan 8, 2024

View reviewed changes

sylwiaszunejko mentioned this pull request Jan 8, 2024

Update driver to populate tablet metadata for request routing #281

Closed

sylwiaszunejko requested a review from Lorak-mmk January 9, 2024 07:18

sylwiaszunejko force-pushed the introduce_tablets branch 6 times, most recently from d075881 to 3ffc22c Compare January 12, 2024 07:28

avelanarius reviewed Jan 12, 2024

View reviewed changes

avelanarius assigned sylwiaszunejko Jan 15, 2024

sylwiaszunejko force-pushed the introduce_tablets branch from 3ffc22c to 4a8f8cb Compare January 15, 2024 15:01

sylwiaszunejko requested a review from avelanarius January 15, 2024 15:01

sylwiaszunejko and others added 3 commits January 15, 2024 16:03

Add integration and unit tests

c3f194b

Add documentation of tablet awareness

eaa9eb1

sylwiaszunejko force-pushed the introduce_tablets branch from 4a8f8cb to eaa9eb1 Compare January 15, 2024 15:03

avelanarius approved these changes Jan 16, 2024

View reviewed changes

avelanarius merged commit dfccfff into scylladb:master Jan 16, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce support for tablets #249

Introduce support for tablets #249

sylwiaszunejko commented Aug 2, 2023 •

edited

Loading

Lorak-mmk left a comment

Lorak-mmk commented Aug 10, 2023

Lorak-mmk left a comment

sylwiaszunejko commented Aug 11, 2023

tgrabiec Aug 14, 2023

sylwiaszunejko Aug 14, 2023

avelanarius Aug 14, 2023

tgrabiec Aug 14, 2023

sylwiaszunejko Aug 14, 2023

sylwiaszunejko commented Jan 5, 2024

Lorak-mmk left a comment

Lorak-mmk Jan 8, 2024

sylwiaszunejko Jan 8, 2024

Lorak-mmk Jan 9, 2024

avelanarius Jan 9, 2024

avelanarius Jan 9, 2024

avelanarius commented Jan 9, 2024

avelanarius commented Jan 9, 2024 •

edited

Loading

sylwiaszunejko commented Jan 12, 2024

avelanarius Jan 12, 2024

sylwiaszunejko Jan 12, 2024

avelanarius Jan 12, 2024

Introduce support for tablets #249

Introduce support for tablets #249

Conversation

sylwiaszunejko commented Aug 2, 2023 • edited Loading

Lorak-mmk left a comment

Choose a reason for hiding this comment

Lorak-mmk commented Aug 10, 2023

Lorak-mmk left a comment

Choose a reason for hiding this comment

sylwiaszunejko commented Aug 11, 2023

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

sylwiaszunejko commented Jan 5, 2024

Lorak-mmk left a 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

avelanarius commented Jan 9, 2024

avelanarius commented Jan 9, 2024 • edited Loading

sylwiaszunejko commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented Aug 2, 2023 •

edited

Loading

avelanarius commented Jan 9, 2024 •

edited

Loading