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

Introduce support for tablets [3.x] #237

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Introduce support for tablets [3.x] #237

merged 1 commit into from
Feb 23, 2024

Conversation

Bouncheck
Copy link
Collaborator

@Bouncheck Bouncheck commented Jul 23, 2023 •
edited by avelanarius
Loading

This PR introduces changes to the driver that are necessary for
shard-awareness and token-awareness to work effectively with the
tablets feature recently introduced to ScyllaDB. It overwrites
the ring-based replica calculations on tablet-enabled keyspaces.

Now if driver sends 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.

This tablet information is then stored in the driver and is used
for correctly routing all next requests.

@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from 55ae641 to 6b6004c Compare July 31, 2023 10:51
@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from 6b6004c to 2fce280 Compare August 14, 2023 11:32
@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from 2fce280 to fedd5e0 Compare August 22, 2023 15:21
@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from fedd5e0 to 9956aee Compare January 27, 2024 16:30
@Bouncheck Bouncheck changed the title Scylla 3.x tablets 3 [3.x] Tablets support Jan 27, 2024
@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from 9956aee to 439f933 Compare January 27, 2024 16:34
@Bouncheck
Copy link
Collaborator Author

Reworked this completely to use solely tablets-routing-v1 protocol extension.
Removed all previous changes related to parsing full system.tablets table.

@Bouncheck Bouncheck marked this pull request as ready for review January 27, 2024 16:37
@Bouncheck Bouncheck requested a review from avelanarius January 27, 2024 16:37
@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch 2 times, most recently from 8807dab to 0d72142 Compare February 6, 2024 11:11
@Bouncheck
Copy link
Collaborator Author

Fixed silent NPE I missed before

avikivity
avikivity reviewed Feb 6, 2024
View reviewed changes
driver-core/src/main/java/com/datastax/driver/core/HostConnectionPool.java
if (keyspace != null && table != null) {
assert t instanceof Token.TokenLong64;
shardId = metadata.getShardForTabletToken(keyspace, table, (Token.TokenLong64) t, host);
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add protection here against too-large shardId.

Scenario:

  1. we get routing information for a tablet
  2. node restarts with fewer shards
  3. we look up routing information, get a shard that no longer exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed 'this' to min(shardCount, 'this')

avikivity
avikivity reviewed Feb 6, 2024
View reviewed changes
driver-core/src/main/java/com/datastax/driver/core/RequestHandler.java
if (defs != null && defs.size() > 0) {
statementTable = defs.getTable(0);
}

Copy link
Member

Choose a reason for hiding this comment

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

You could capture the tablet map during prepare time and put it into statement itself, avoiding all the casting and checks here. Probably negligible in terms of performance.

@Bouncheck Bouncheck force-pushed the scylla-3.x-tablets-3 branch from c693eff to 78ec7f1 Compare February 22, 2024 18:41
@avelanarius avelanarius changed the title [3.x] Tablets support Introduce support for tablets [3.x] Feb 23, 2024
@Bouncheck @avelanarius
Introduce support for tablets
6c6052d 
This PR introduces changes to the driver that are necessary for
shard-awareness and token-awareness to work effectively with the
tablets feature recently introduced to ScyllaDB. It overwrites
the ring-based replica calculations on tablet-enabled keyspaces.

Now if driver sends 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.

This tablet information is then stored in the driver and is used
for correctly routing all next requests.
@avelanarius
Copy link

As for additional manual testing:

  • I ran c-s with this version of the driver combined with tablet_allocator_shuffle (one_shot=False so it was constantly moving tablets around). The driver worked without any errors for the entire (1h) duration.
  • I ran longevity-100gb-4h-test with c-s with this version of the driver and it passed correctly.
  • To test the behavior in case of having stale data (of nodes that no longer exist), I created a 2-node cluster, started the client and ran some queries (to populate tablet metadata) and then I added additional 2 nodes and removed the original nodes. The driver correctly handled the situation where it had entirely stale data about all tablets and wasn't fooled by non-existing nodes.
  • To verify that there is no regression in performance, I compared the performance of cassandra-stress with and without this PR. The performance with this PR was improved (better throughput, slightly better latencies) compared to without this PR - I won't share the specific numbers as the goal was to sanity check that there is no major regression, not to properly benchmark the driver (which would take more time to do properly).

@avelanarius avelanarius merged commit dbfe82a into scylladb:scylla-3.x Feb 23, 2024
11 of 13 checks passed
avelanarius added a commit to avelanarius/scylla-tools-java that referenced this pull request Feb 27, 2024
@avelanarius
build.xml: update scylla-driver-core to 3.11.5.2
f7ed750 
ScyllaDB Java Driver 3.11.5.2 supports tablet awareness in the driver.
Update the version in scylla-tools-java so that cassandra-stress can
take advantage of this new functionality.

Before releasing Java Driver 3.11.5.2 we ran several tests with
cassandra-stress to make sure that it's stable (under tablet migration,
topology changes) and that there is no performance regression.
(more details here: scylladb/java-driver#237 (comment))
avelanarius added a commit to avelanarius/scylla-tools-java that referenced this pull request Feb 27, 2024
@avelanarius
build.xml: update scylla-driver-core to 3.11.5.2
7969fc7 
ScyllaDB Java Driver 3.11.5.2 adds tablet awareness to the driver.
Update the version of the driver in scylla-tools-java so that 
cassandra-stress can take advantage of this new functionality.

Before releasing Java Driver 3.11.5.2 we ran several tests with
cassandra-stress to make sure that it's stable (under tablet migration,
topology changes) and that there is no performance regression.
(more details here: scylladb/java-driver#237 (comment))
@avelanarius avelanarius mentioned this pull request Feb 27, 2024
Closed
denesb pushed a commit to scylladb/scylla-tools-java that referenced this pull request Mar 8, 2024
@avelanarius @denesb
build.xml: update scylla-driver-core to 3.11.5.2
d61296d 
ScyllaDB Java Driver 3.11.5.2 adds tablet awareness to the driver.
Update the version of the driver in scylla-tools-java so that
cassandra-stress can take advantage of this new functionality.

Before releasing Java Driver 3.11.5.2 we ran several tests with
cassandra-stress to make sure that it's stable (under tablet migration,
topology changes) and that there is no performance regression.
(more details here: scylladb/java-driver#237 (comment))

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

@avikivity avikivity avikivity left review comments

@avelanarius avelanarius avelanarius approved these changes

Assignees

@Bouncheck Bouncheck

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bouncheck @avelanarius @avikivity