feat: Add Apache Cassandra Storage Driver#1665
Conversation
WalkthroughThe pull request introduces a new Cassandra storage implementation. The main README now includes an entry for Cassandra with a GitHub Actions badge. A dedicated Cassandra documentation file explains the driver’s installation, usage, configuration, and examples. Additionally, new Go files implement the Cassandra storage driver with methods for CRUD operations, keyspace and table setup, TTL handling, and proper connection management. Integration tests using Testcontainers validate basic operations, key expiration, reset functionality, concurrent access, and identifier validation. A configuration file defines defaults and enables customization of the driver’s settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Storage
participant CassandraDB
Client->>Storage: New(config)
Storage->>CassandraDB: Verify/Create keyspace & table
Client->>Storage: Set(key, value, TTL)
Storage->>CassandraDB: Insert key-value with TTL
Client->>Storage: Get(key)
Storage->>CassandraDB: Query key-value
CassandraDB-->>Storage: Return value or indicate expiration
Storage-->>Client: Return result
Client->>Storage: Delete(key) / Reset()
Storage->>CassandraDB: Execute delete/reset commands
CassandraDB-->>Storage: Confirmation
Storage-->>Client: Acknowledge operation
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
cassandra/config.go (1)
41-42: Remove duplicated comment.The comment on line 41 "ConfigDefault is the default config" is a duplicate of the one on line 31. This appears to be a copy-paste error.
-// ConfigDefault is the default config +// Helper function to apply default config func configDefault(config ...Config) Config {cassandra/README.md (5)
3-3: URL and text mismatch in the description.The link URL points to "github.com/gocql/gocql" but the text displays "github.com/apache/cassandra-gocql-driver", which could be confusing for users.
-A Cassandra storage driver using [https://github.com/gocql/gocql](https://github.com/apache/cassandra-gocql-driver). +A Cassandra storage driver using [gocql/gocql](https://github.com/gocql/gocql).
9-15: Fix heading hierarchy issue.The document jumps from H1 (single #) to H3 (three ###) without using H2 (double ##) first, which violates proper Markdown heading hierarchy.
-### Table of Contents +## Table of Contents🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
74-94: Replace hard tabs with spaces in Markdown code block.The markdown linter has detected hard tabs in the code block, which can cause inconsistent rendering across platforms.
Consider replacing tabs with spaces in the code block for consistent rendering across different Markdown viewers.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
100-107: Replace hard tabs with spaces in Markdown code block.Similar to the previous code block, hard tabs have been detected here as well.
Consider replacing tabs with spaces in this code block too.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
101-101: Hard tabs
Column: 1(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-51: Specify Docker tag variable.The Docker command uses a placeholder "tag" which might confuse users.
-docker run --name cassandra --network host -d cassandra:tag +docker run --name cassandra --network host -d cassandra:4.1.3This matches the version used in the tests.
cassandra/cassandra_test.go (2)
53-55: Provide reason for skipping test.The keyspace creation test is skipped without explaining why. Add a comment explaining the reason for skipping this test to improve maintainability.
- t.Run("KeyspaceCreation", func(t *testing.T) { - t.Skip("Skipping keyspace creation test") - testKeyspaceCreation(t, connectionURL) + t.Run("KeyspaceCreation", func(t *testing.T) { + // TODO: Fix keyspace creation test - currently skipped because it conflicts with + // other tests or requires special setup + t.Skip("Skipping keyspace creation test") + testKeyspaceCreation(t, connectionURL)
37-72: Consider adding environment variable for disabling integration tests.Long-running integration tests with external dependencies like this could be conditionally enabled to speed up testing during development.
You might want to add a conditional skip based on an environment variable, for example:
func TestCassandraStorage(t *testing.T) { + if os.Getenv("SKIP_CASSANDRA_INTEGRATION_TESTS") == "true" { + t.Skip("Skipping Cassandra integration tests") + } ctx := context.Background() // ... rest of the testcassandra/cassandra.go (6)
12-19: Store TTL as time.Duration for improved precision
Thettlfield is stored as anint, which might lead to precision loss or overflow with large durations. You might consider storing TTL as atime.Durationto better represent the range of potential expiration values.
60-101: Potential performance impact of dropping tables during reset
If multiple instances attempt to create or reset the keyspace concurrently, you may run into conflicts or performance bottlenecks. Consider a more granular approach for partial resets or verifying concurrency usage to avoid downtime.
103-127: Use NetworkTopologyStrategy for better real-world resilience
TheSimpleStrategywithreplication_factor=1is typically suitable for local or development environments only. In multi-data-center production setups, it is recommended to useNetworkTopologyStrategywith appropriate replication factors.- "CREATE KEYSPACE %s WITH REPLICATION = {'class': 'SimpleStrategy', 'replication_factor': 1}", + "CREATE KEYSPACE %s WITH REPLICATION = { + 'class': 'NetworkTopologyStrategy', + 'datacenter1': 3 + }",
129-141: Good basic schema for key-value usage
The table creation is minimal and sufficient for a basic key-value store. Consider additional columns or partitioning strategies if you anticipate large-scale usage or advanced query patterns.
188-212: Fallback expiration check
This logic ensures that expired items are removed if they persist beyond their TTL. For higher scale, consider background tasks or server-side eviction to avoid potential overhead on reads.
235-417: Commented-out test code
The commented-out integration tests appear thorough and would be helpful for ensuring functionality. Consider enabling them or placing them in a dedicated test file if you still need them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-cassandra.ymlis excluded by!**/*.ymlcassandra/go.modis excluded by!**/*.modcassandra/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
README.md(1 hunks)cassandra/README.md(1 hunks)cassandra/cassandra.go(1 hunks)cassandra/cassandra_test.go(1 hunks)cassandra/config.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cassandra/cassandra_test.go (2)
cassandra/cassandra.go (1)
New(30-58)cassandra/config.go (1)
Config(10-29)
cassandra/cassandra.go (1)
cassandra/config.go (1)
Config(10-29)
🪛 markdownlint-cli2 (0.17.2)
cassandra/README.md
9-9: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1
(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1
(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (17)
cassandra/config.go (3)
10-29: Struct design and documentation looks good.The Config struct is well-defined with clear documentation for each field, including whether they're optional and what their default values are.
32-39: Default configuration is appropriate.The default configuration provides sensible values that should work for most basic setups.
42-73: Implementation of configDefault is robust.The implementation correctly handles the case when no config is provided, and it properly applies defaults for each field that isn't specified.
README.md (1)
57-57: Storage implementation added correctly.The Cassandra implementation has been correctly added to the list of storage implementations with appropriate linking and badge format, consistent with other implementations.
cassandra/README.md (1)
16-29: Config documentation is comprehensive.The README thoroughly documents the Config struct, its fields, and default values, which aligns well with the actual implementation in config.go.
Also applies to: 71-95, 97-108
cassandra/cassandra_test.go (6)
183-184: Consider flakiness with time-based tests.Using
time.Sleep()for testing expiration can lead to flaky tests, especially in CI environments where resources might be constrained.While time.Sleep is sometimes necessary for testing time-based operations, consider using slightly longer durations or adding a small buffer to reduce flakiness in different environments. You might also consider a more deterministic approach if possible.
Also applies to: 199-200
16-35: Good container setup approach using testcontainers.Using testcontainers for the integration tests is a great approach, as it ensures tests run against a real Cassandra instance. The setup function is well-structured and handles error cases appropriately.
112-143: Basic operations test is comprehensive.The basic operations test correctly verifies all the essential CRUD operations including handling of non-existent keys.
146-209: Expirable keys test is thorough.The test for key expiration is well-designed, testing different TTL scenarios:
- Default TTL
- Specific TTL
- No TTL (overriding default)
The assertions correctly verify the expected behavior in each case.
211-264: Reset functionality test is well-structured.The test properly verifies both the
Reset()method and theResetflag in the config, ensuring data is cleared in both scenarios.
266-310: Good test for concurrent access.The concurrent access test correctly tests the thread safety of the storage implementation by performing operations from multiple goroutines. The synchronization with a channel is a good approach.
cassandra/cassandra.go (6)
21-27: Unused SchemaInfo struct
It appears thatSchemaInfois declared but not used anywhere in this file. If it is intended for future features, you can keep it. Otherwise, removing the unused code helps maintain cleanliness.
143-154: Dropping 'schema_info' table without creation
The code callsDROP TABLE IF EXISTS %s.schema_info, but there's no corresponding function in this file that creates or manages it. Confirm if you have creation code elsewhere, or remove references if it's no longer needed.
156-186: Compact and efficient TTL handling
UsingINSERTalongsideUSING TTLis efficient. The approach to fallback to a default TTL or indefinite storage is consistent with Cassandra best practices.
214-218: Straightforward deletion
The key deletion logic here aligns with typical Cassandra usage for key-value data.
220-224: Simple table reset
Truncating the table is a clear and direct approach to remove all keys at once.
226-231: Proper resource cleanup
Closing the session avoids potential connection leaks. The approach is correct for gracefully shutting down your Cassandra connection.
mdelapenya
left a comment
There was a problem hiding this comment.
Added a few comments, thank you @MitulShah1 for your hard work with the review, much appreciated 🙇
cassandra/cassandra_test.go
Outdated
| b.ReportAllocs() | ||
| b.ResetTimer() |
There was a problem hiding this comment.
suggestion: let's move this right before the for loop and the err declaration (see #1672)
|
@mdelapenya @gaby ready to merge? |
mdelapenya
left a comment
There was a problem hiding this comment.
Added some comments regarding the readability of the test code. No blockers, so I'm open to discussion here.
Thanks @MitulShah1 for your patience during the review 🙇
Thank you for the feedback! 🙇 I understand the importance of consistency and readability in tests, and I agree that clarity often trumps duplication in such cases. In this case, I used the built-in testing functionality to keep the code concise and idiomatic, especially since this is a new function and not used elsewhere in the repo yet. That said, I appreciate the perspective and have updated the code to reflect the more explicit pattern used in other modules to maintain consistency. Thanks again for pointing it out—happy to adapt for the sake of alignment across the codebase. |
mdelapenya
left a comment
There was a problem hiding this comment.
Final review, found something regarding the imports
After that, we are ready to go
Thanks for update. |
mdelapenya
left a comment
There was a problem hiding this comment.
In regards Testcontainers code, LGTM, thanks!
| ) | ||
|
|
||
| // Config defines the configuration options for the Cassandra storage | ||
| type Config struct { |
There was a problem hiding this comment.
There's one major thing missing here, TLSConfig support
See https://pkg.go.dev/github.com/gocql/gocql#hdr-Transport_layer_security
There was a problem hiding this comment.
Probably worth adding a config for this too https://pkg.go.dev/github.com/gocql/gocql#hdr-Data_center_awareness_and_query_routing
|
@MitulShah1 @mdelapenya Only thing missing is being able to pass TLSConfig. |
Cassandra module in tc-go does not support TLS... yet 😉 |
That's fine, we can add the config option for now. Tests later? |
Sorry didn't get you, means add option in Config but did not use in code you mean? |
Add option and use it, don't worry about creating a test that uses TLS yet. We can do that in another PR in the future. |
|
@MitulShah1 Can you update the README.md with the new options in the config section. Thanks! |
gaby
left a comment
There was a problem hiding this comment.
👍 LGTM, only thing missing is updating the README.md
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cassandra/README.md (2)
5-5: Use “latest release” badge for consistency.
The current badge uses/v/tag/...with alt text “Release.” As previously suggested, it’s clearer to reference the latest release. Consider switching to the/releases/latestendpoint or updating the alt text to “Latest release.”
17-27: Remove or implement theConnmethod to avoid API drift.
The docs list:func (s *Storage) Conn() *gocql.Sessionbut the codebase does not yet implement this accessor. Please either add the
Conn()method to return the underlying session or remove it from the README.
🧹 Nitpick comments (2)
cassandra/README.md (2)
1-1: Consider clarifying the title.
Users may not immediately recognize “Cassandra” in this context as Apache Cassandra. I suggest updating the heading to:# Apache Cassandrafor better clarity.
61-70: Add TLSConfig usage to the example if applicable.
Given the planned support for TLS configuration, it would be helpful to illustrate how to pass a customTLSConfig(e.g.,tls.Config) in the example. This will guide users on securing their Cassandra connections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cassandra/README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: compare (cassandra)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
🔇 Additional comments (3)
cassandra/README.md (3)
9-16: The table of contents accurately reflects the document structure.
29-37: Installation instructions look good.
Thego get github.com/gofiber/storage/cassandracommand correctly installs the new driver.
39-53: Integration test instructions are clear.
The Testcontainers guidance and the Docker command (docker run --name cassandra -p 9042:9042 -d cassandra:latest) align with project conventions.
| var ConfigDefault = Config{ | ||
| Hosts: []string{"localhost:9042"}, | ||
| Keyspace: "gofiber", | ||
| Table: "kv_store", | ||
| Consistency: gocql.Quorum, | ||
| Reset: false, | ||
| Expiration: 10 * time.Minute, | ||
| MaxRetries: 3, | ||
| ConnectTimeout: 5 * time.Second, | ||
| SslOpts: nil, | ||
| PoolConfig: &gocql.PoolConfig{ | ||
| HostSelectionPolicy: gocql.TokenAwareHostPolicy(gocql.RoundRobinHostPolicy()), | ||
| }, | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default for the new TLSConfig field.
If you introduce TLSConfig to Config, ensure it’s set to nil in ConfigDefault for backward compatibility.
| ```go | ||
| // Config defines the configuration options for the Cassandra storage | ||
| type Config struct { | ||
| // Optional. Default is localhost | ||
| // Hosts is a list of Cassandra nodes to connect to. | ||
| Hosts []string | ||
| // Optional. Default is gofiber | ||
| // Keyspace is the name of the Cassandra keyspace to use. | ||
| Keyspace string | ||
| // Optional. Default is kv_store | ||
| // Table is the name of the Cassandra table to use. | ||
| Table string | ||
| // Optional. Default is Quorum | ||
| // Consistency is the Cassandra consistency level. | ||
| Consistency gocql.Consistency | ||
| // Optional. PoolConfig.HostSelectionPolicy = gocql.TokenAwareHostPolicy(gocql.RoundRobinHostPolicy()) | ||
| // PoolConfig is the Cassandra connection pool configuration. | ||
| PoolConfig *gocql.PoolConfig | ||
| // Optional. Default is false | ||
| // SslOpts is the SSL options for the Cassandra cluster. | ||
| SslOpts *gocql.SslOptions | ||
| // Optional. Default is 10 minutes | ||
| // Expiration is the time after which an entry is considered expired. | ||
| Expiration time.Duration | ||
| // Optional. Default is false | ||
| // Reset is a flag to reset the database. | ||
| Reset bool | ||
| // Optional. Default is 3 | ||
| // MaxRetries is the maximum number of retries for a query. | ||
| MaxRetries int | ||
| // Optional. Default is 5 seconds | ||
| // ConnectTimeout is the timeout for connecting to the Cassandra cluster. | ||
| ConnectTimeout time.Duration | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose a TLSConfig field in the Config struct.
Per the prior discussion, even if immediate TLS tests are deferred, adding a TLSConfig *tls.Config option (alongside or in place of SslOpts) ensures users can supply custom TLS settings. Please update the struct accordingly.
Done |
|
Fantastic job! @MitulShah1 is willing to add TLS support to the testcontainers' Cassandra module 🚀 |
Added Apache Cassandra Storage Driver with Apache Go Driver.
Summary by CodeRabbit
New Features
Documentation
Tests