feat: Support Keyspace #2454

PeterChen13579 · 2025-08-18T20:28:55Z

Support AWS Keyspace queries

codecov · 2025-08-19T02:48:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.97%. Comparing base (3f2ada3) to head (497721e).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2454   +/-   ##
========================================
  Coverage    79.96%   79.97%           
========================================
  Files          384      384           
  Lines        15935    15936    +1     
  Branches      8340     8341    +1     
========================================
+ Hits         12743    12745    +2     
+ Misses        1912     1910    -2     
- Partials      1280     1281    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

docs/config-description.md

src/data/CassandraBackend.hpp

src/data/cassandra/impl/Batch.cpp

src/data/cassandra/impl/Cluster.cpp

kuznetsss

LGTM, but let's wait for an approval from @godexsoft

godexsoft

I have not thoroughly reviewed the code because i don't yet understand why we need so many changes if keyspaces is supposed to be compatible with cassandra?
In general i'm not a fan of adding custom stuff to the cassandra backend.. if we need to have custom behaviour maybe we should create a separate backend and inherit where we can, modify where we need.

godexsoft · 2025-08-27T17:33:14Z

src/data/CassandraBackend.hpp

        if (!range_) {
-            executor_.writeSync(schema_->updateLedgerRange, ledgerSequence_, false, ledgerSequence_);
+            executor_.writeSync(schema_->insertLedgerRange, false, ledgerSequence_);
+            executor_.writeSync(schema_->insertLedgerRange, true, ledgerSequence_);


Why was this not needed before, do you know?

Because before, the UpdateLedgerRange would insert the first range that it finds, let's say rippled started at ledger 256; this would insert 256, and the false means it is not the latest ledger in our ledger_range table.
Since the updateLedgerRange can't be used in Keyspace, I simplified it to both keyspace and scylla using this insertLedgerRange schema, as I felt like it is easier to understand the logic behind it. (ie, first time clio loads up without a ledger range table, insert both the latest and non latest range into the table)

godexsoft · 2025-08-27T17:34:26Z

docs/config-description.md

+- **Required**: True
+- **Type**: string
+- **Default value**: `cassandra`
+- **Constraints**: The value must be one of the following: `cassandra`, `aws_keyspace`.


May as well add scylladb to the mix then.. if we are using the same backend for multiple DBs already

I think the reason we didn't add scylladb is because scylladb and cassandra would be the same as it uses fully the same schemas

godexsoft · 2025-08-27T17:37:43Z

src/data/cassandra/Schema.hpp

+                    );
+                }();
+                // AWS_keyspace supported queries
+            } else if (settingsProvider_.get().getSettings().provider == "aws_keyspace") {


Please can you explain why we need so many changes and what exactly is not supported so we had to do this?

Explained in my other comment 👍

godexsoft · 2025-08-27T17:38:52Z

src/data/cassandra/Schema.hpp

        PreparedStatement updateLedgerRange = [this]() {
            return handle_.get().prepare(
                fmt::format(
                    R"(
                UPDATE {}
                   SET sequence = ?
                 WHERE is_latest = ?
-                    IF sequence IN (?, null)


This change was suggested by scylla team before IIRC.. this gave us a good speedup - let's not go back to the slower old version

I can revert the logic so that scylladb uses this 👍

PeterChen13579 · 2025-08-27T19:32:13Z

@godexsoft For some context, Keyspace is compatible with Cassandra but there's a few limitations. Specifically the places where we had:

IF ... IN clause 
PER PARTITION LIMIT 1 
Tuple comparisons, e.g.  (taxon, token_id) > ?

To tackle this, I split the queries that contains the above into 2 queries. ie, 2 queries will be equivalent to the 1 from before.

The goal of this PR is to get Clio to run with Keyspace while those running cassandra/scylladb would work the exact same.
Also, keyspace is trying to support the above statements that they don't support right now. Hopefully once they get it supported, I'm happy to revert this PR.

mathbunnyru

Left some comments to mostly make the code more readable and less error-prone

docs/config-description.md

src/data/CassandraBackend.hpp

mathbunnyru · 2025-08-28T11:02:43Z

src/data/CassandraBackend.hpp

+    ) const
+    {
+        std::vector<ripple::uint256> nftIDs;
+        if (settingsProvider_.getSettings().provider == "aws_keyspace") {


Let's have 2 private methods, and here just call one or another under if

Why was this marked as resolved?

After discussing with @kuznetsss and @godexsoft , we are completely changing the structure of the backend, (ie, adding a cassandraFamily level where CassandraBackend and KeyspaceBackend is inheriting from) there's not going to be anymore checking provider in backend, so I marked it as resolved.

Ok, please, don't just "resolve" the issue, without any comment 😂

Keep it open until it's actually resolved, even if it's resolved not the way that reviewer (in this case me) suggested

mathbunnyru · 2025-08-28T11:03:18Z

src/data/cassandra/Schema.hpp

@@ -347,6 +348,86 @@ class Schema {
        Statements(SettingsProviderType const& settingsProvider, Handle const& handle)
            : settingsProvider_{settingsProvider}, handle_{std::cref(handle)}
        {
+            // initialize scylladb supported queries
+            if (settingsProvider_.get().getSettings().provider == "scylladb") {


Let's also have 2 private methods here

Why was this marked as resolved?

mathbunnyru · 2025-08-28T11:06:57Z

src/data/cassandra/Schema.hpp

+        // For ScyllaDB / Cassandra ONLY
+        std::optional<PreparedStatement> selectAccountFromBeginningScylla;
+        std::optional<PreparedStatement> selectAccountFromTokenScylla;
+        std::optional<PreparedStatement> selectNFTsByIssuerScylla;
+
+        // For AWS Keyspaces ONLY
+        // NOTE: AWS keyspace is not able to load cache with accounts
+        std::optional<PreparedStatement> selectNFTsAfterTaxonKeyspaces;


Should be std::variant<CassandraStatements, AWSKeyspacesStatements>, and each one is a struct of needed statements, you will probably be able to get rid of std::optional as well

Why was this marked resolved?

src/data/cassandra/impl/Cluster.cpp

src/data/cassandra/impl/Cluster.hpp

mathbunnyru · 2025-08-28T11:11:44Z

src/data/cassandra/impl/Cluster.hpp

@@ -88,6 +89,9 @@ struct Settings {
    /** @brief Size of batches when writing */
    std::size_t writeBatchSize = kDEFAULT_BATCH_SIZE;

+    /** @brief Provider to know if we are using scylladb or keyspace */
+    std::string provider = kDEFAULT_PROVIDER;


Let's create an enum for the provider.
I know that the config definition doesn't support the enumeration, but I think we shouldn't use raw strings and raw strings comparison in the code itself, because it's error-prone (see my comment above for incorrect kDEFAULT_PROVIDER value)

tests/integration/data/BackendFactoryTests.cpp

PeterChen13579 added 2 commits August 18, 2025 15:38

feat: Support Keyspace

bbd2884

remove unnecessary comment

d598396

kuznetsss reviewed Aug 19, 2025

View reviewed changes

docs/config-description.md Outdated Show resolved Hide resolved

src/data/CassandraBackend.hpp Show resolved Hide resolved

src/data/cassandra/impl/Batch.cpp Show resolved Hide resolved

src/data/cassandra/impl/Cluster.cpp Outdated Show resolved Hide resolved

PeterChen13579 and others added 6 commits August 19, 2025 16:19

fix bug and comments

c3e0442

Merge branch 'develop' into SupportKeyspace

47c2af0

fix comments

0d9a83f

merge develop

8f6bec2

fix val

30da8d8

Merge branch 'develop' into SupportKeyspace

e03f5e4

PeterChen13579 requested a review from kuznetsss August 27, 2025 14:56

kuznetsss requested a review from godexsoft August 27, 2025 15:08

kuznetsss reviewed Aug 27, 2025

View reviewed changes

godexsoft reviewed Aug 27, 2025

View reviewed changes

let scylladb use same schema to update ledger range as before

f88ce31

mathbunnyru requested changes Aug 28, 2025

View reviewed changes

mathbunnyru and others added 5 commits August 28, 2025 15:11

Merge branch 'develop' into SupportKeyspace

84db880

Merge branch 'develop' into SupportKeyspace

8bc36c2

Merge branch 'develop' into SupportKeyspace

fc88abd

fix comments

2653010

change connections per host

497721e

feat: Support Keyspace #2454

Are you sure you want to change the base?

feat: Support Keyspace #2454

Uh oh!

Conversation

PeterChen13579 commented Aug 18, 2025

Uh oh!

codecov bot commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kuznetsss left a comment

Choose a reason for hiding this comment

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterChen13579 commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathbunnyru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 19, 2025 •

edited

Loading

PeterChen13579 commented Aug 27, 2025 •

edited

Loading