feat(scylladb): add scylladb provider #2919

danielhe4rt · 2024-12-13T16:29:45Z

What does this PR do?

Implements the Database ScyllaDB as a new provider.

Under the ScyllaDBContainer you will have the possibility of:

Enable Shard Awareness port using WithShardAwareness;
Set Docker Start Commands with WithCommand
Fetch ports with ConnectionHost
Enable ScyllaDB Alternator Endpoint (DynamoDB Compatible API)

API Example

ctx := context.Background()

ctr, err := scylladb.Run(ctx,
   "scylladb/scylla:6.2",
   scylladb.WithConfigFile(filepath.Join("testdata", "scylla.yaml")),
   scylladb.WithShardAwareness(),
   //...
)

Why is it important?

ScyllaDB is a NoSQL Database that is being constantly used by big companies and acts as a Drop-in Replacement to CassandraDB and DynamoDB.

How to test this PR

Clone the repository and enter the scylladb module folder:

git clone https://github.com/basementdevs/testcontainers-go basement-testcontainers-go
cd basement-testcontainers-go/modules/scylladb
make test

netlify · 2024-12-13T16:30:06Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`aabf4d9`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6781712653ac6600080fbe71
😎 Deploy Preview	https://deploy-preview-2919--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…wareness

…rd-awareness port

stevenh

Thanks for a PR @danielhe4rt, have done an initial review with some suggestions and questions for you.

stevenh · 2024-12-15T12:34:20Z

modules/scylladb/scylladb.go

+	shardAwarePort = "19042/tcp"
+)
+
+type Container struct {


suggestion: Add a doc comment to describe.

stevenh · 2024-12-15T12:38:06Z

modules/scylladb/scylladb.go

+		cf := testcontainers.ContainerFile{
+			HostFilePath:      configFile,
+			ContainerFilePath: "/etc/scylla/scylla.yaml",
+			FileMode:          0o755,


suggestion: no need for the file to be executable

modules/scylladb/scylladb.go

stevenh · 2024-12-15T12:43:42Z

modules/scylladb/scylladb.go

+// WithAlternator enables the Alternator (DynamoDB Compatible API) service in the ScyllaDB container.
+// It will set the "alternator-port" parameter to the specified port.
+// It will also set the "alternator-write-isolation" parameter to "always" as a command line argument to the container.
+func WithAlternator(alternatorPort int) testcontainers.CustomizeRequestOption {


suggestion: use the in type which matches a port definition

Suggested change

func WithAlternator(alternatorPort int) testcontainers.CustomizeRequestOption {

func WithAlternator(alternatorPort uint16) testcontainers.CustomizeRequestOption {

I think having WithAlternator() would be enough, simplifying user setup. Regarding to alternator-write-isolation, is always recommended for testing?

depend on the application. for testing it's "slower" but you probably wont be doing stress testing in the cluster.

stevenh · 2024-12-15T12:44:57Z

modules/scylladb/scylladb.go

+// ConnectionHost returns the host and port of the Scylladb container with the default port
+// and obtaining the host and exposed port from the container
+// Eg: "host:port" -> "localhost:9042" -> "localhost:19042" -> "localhost:8000"
+func (c Container) ConnectionHost(ctx context.Context, port int) (string, error) {


suggestion: use uint16 to match port requirement.

Suggested change

func (c Container) ConnectionHost(ctx context.Context, port int) (string, error) {

func (c Container) ConnectionHost(ctx context.Context, port uint16) (string, error) {

stevenh · 2024-12-15T12:50:50Z

modules/scylladb/scylladb_test.go

+	testcontainers.CleanupContainer(t, ctr)
+	require.NoError(t, err)
+
+	t.Run("test without shard awareness", func(t *testing.T) {


suggestion: we're standardising on simple names without spaces to ensure they match the output

Suggested change

t.Run("test without shard awareness", func(t *testing.T) {

t.Run("without-shard-awareness", func(t *testing.T) {

More below

stevenh · 2024-12-15T12:53:39Z

modules/scylladb/scylladb_test.go

+
+	ctr, err := scylladb.Run(ctx,
+		"scylladb/scylla:6.2",
+		scylladb.WithConfigFile(filepath.Join("testdata", "scylla.yaml")),


suggestion: with the change to reader, you can leverage embed

//go:embed testdata/scylla.yaml var testConfig []byte

Suggested change

scylladb.WithConfigFile(filepath.Join("testdata", "scylla.yaml")),

scylladb.WithConfig(bytes.NewReader(testConfig)),

stevenh · 2024-12-15T12:54:35Z

modules/scylladb/scylladb_test.go

+	})
+}
+
+func TestScyllaWithAlternator(t *testing.T) {


question: These next two tests look like duplicates of the above, or could be combined?

stevenh · 2024-12-15T12:55:50Z

modules/scylladb/scylladb_test.go

+	return dynamodb.NewFromConfig(cfg, dynamodb.WithEndpointResolverV2(&scyllaAlternatorResolver{HostPort: hostPort})), errors.Join(errs...)
+}
+
+func createTable(client *dynamodb.Client) error {


suggestion: pass in t and require.NoError as thats the only use of this.

stevenh · 2024-12-15T12:56:16Z

modules/scylladb/testdata/scylla.yaml

+murmur3_partitioner_ignore_msb_bits: 12
+strict_is_not_null_in_views: true
+maintenance_socket: ignore
+enable_tablets: true


suggestion: add trailing newline.

eddumelendez · 2024-12-16T22:06:46Z

modules/scylladb/scylladb_test.go

@@ -0,0 +1,194 @@
+package scylladb_test


Can we include a test using ssl? I think would be nice to have.

eddumelendez · 2024-12-17T22:04:35Z

modules/scylladb/scylladb_test.go

+		host, err := ctr.ConnectionHost(ctx, 19042)
+		require.NoError(t, err)
+
+		cluster := gocql.NewCluster(host)
+		session, err := cluster.CreateSession()
+		require.NoError(t, err)
+		defer session.Close()
+
+		var address string
+		err = session.Query("SELECT address FROM system.clients").Scan(&address)
+		require.NoError(t, err)
+	})
+
+	t.Run("test with shard awareness", func(t *testing.T) {
+		host, err := ctr.ConnectionHost(ctx, 19042)
+		require.NoError(t, err)
+
+		cluster := gocql.NewCluster(host)
+		session, err := cluster.CreateSession()
+		require.NoError(t, err)
+		defer session.Close()
+
+		var address string
+		err = session.Query("SELECT address FROM system.clients").Scan(&address)
+		require.NoError(t, err)
+	})


I see both tests are the same. should one of them use 9042 instead? Is it enough testing the connection?

eddumelendez · 2024-12-17T22:10:21Z

modules/scylladb/scylladb.go

+}
+
+// WithShardAwareness enable shard-awareness in the ScyllaDB container so you can use the `19042` port.
+func WithShardAwareness() testcontainers.CustomizeRequestOption {


scylladb starts both ports 9042 and 19042 by default. IMO we should expose a fn to get the mapped port for 19042 and omit this one.

@danielhe4rt @eddumelendez I'm taking over the PR and adding the suggestions on top of the original PR.

I'd like to have this in the upcoming release.

Thanks all for your time here

Ups, I noticed this PR is sent from the main branch of the fork, so unfortunately I cannot contribute commits to it.

@danielhe4rt could you please address the above comments 🙏 ?

Done! Sorry for the delay. Tomorrow I'll sync with @CodeLieutenant and finish it for good. Give us one more day @mdelapenya. We're also gonna stream it, so you can find us on twitch.tv/danielhe4rt.

Co-authored-by: Steven Hartland <[email protected]>

mdelapenya

@danielhe4rt thanks for your time in this PR, I added a few comments/suggestions in the review.

Could you address them 🙏 ?

Thank you in advance

mdelapenya · 2025-01-10T10:18:38Z

docs/modules/scylladb.md

+- Not available until the next release of testcontainers-go <a href="https://github.com/testcontainers/testcontainers-go"><span class="tc-version">:material-tag: main</span></a>
+
+In the case you have a custom config file for ScyllaDB, it's possible to copy that file into the container before it's
+started, using the `WithConfigFile(cfgPath string)` function.


suggestion: let's use the WithConfig(io.Reader) method here, as suggested in the review comments. It's also applied to the existing tests, so we should change the method call in there.

mdelapenya · 2025-01-10T10:19:07Z

docs/modules/scylladb.md

+[With Config YAML](../../modules/scylladb/examples_test.go) inside_block:runScyllaDBContainerWithConfigFile
+<!--/codeinclude-->
+!!!warning
+    You should provide a valid ScyllaDB configuration file when using the function, otherwise the container will fail to


suggestion: see above

Suggested change

You should provide a valid ScyllaDB configuration file when using the function, otherwise the container will fail to

You should provide a valid ScyllaDB configuration file as an `io.Reader` when using the function, otherwise the container will fail to

mdelapenya · 2025-01-10T10:19:24Z

docs/modules/scylladb.md

+!!! info
+    By default, we add the `--developer-mode=1` flag to the ScyllaDB container to disable the various checks Scylla
+    performs.
+    Also In scenarios in which static partitioning is not desired - like mostly-idle cluster without hard latency


nit: typo

Suggested change

Also In scenarios in which static partitioning is not desired - like mostly-idle cluster without hard latency

Also in scenarios in which static partitioning is not desired - like mostly-idle cluster without hard latency

mdelapenya · 2025-01-10T10:21:17Z

modules/scylladb/examples_test.go

+
+	fmt.Println(state.Running)
+
+	connectionHost, err := scyllaContainer.ConnectionHost(ctx, 8080) // Alternator port


question: in L61 we are setting the alternator port as 8000. Is this correct?

Suggested change

connectionHost, err := scyllaContainer.ConnectionHost(ctx, 8080) // Alternator port

connectionHost, err := scyllaContainer.ConnectionHost(ctx, 8000) // Alternator port

mdelapenya · 2025-01-10T10:22:22Z

modules/scylladb/examples_test.go

+	// true
+}
+
+func ExampleRun_withConfigFile() {


suggestion: as above, we probably want to use the WithConfig(io.Reader) here

mdelapenya · 2025-01-10T10:26:17Z

modules/scylladb/scylladb.go

+	port           = "9042/tcp"
+	shardAwarePort = "19042/tcp"


question: should we expose these ports so that client code can generate URLs based on them? I see an example test using one of them. If not, we probably want a container method returning the connection string for the given port.

mdelapenya · 2025-01-10T10:27:47Z

modules/scylladb/scylladb.go

+	testcontainers.Container
+}
+
+// WithConfig sets the YAML config file to be used for the ScyllaDB container


Suggested change

// WithConfig sets the YAML config file to be used for the ScyllaDB container

// WithConfig sets the YAML config file as an [io.Reader] to be used for the ScyllaDB container

mdelapenya · 2025-01-10T10:42:41Z

modules/scylladb/scylladb_test.go

+	})
+}
+
+func TestScyllaWithConfigFile(t *testing.T) {


suggestion: as above

Suggested change

func TestScyllaWithConfigFile(t *testing.T) {

func TestScyllaWithConfig(t *testing.T) {

mdelapenya · 2025-01-10T10:44:38Z

modules/scylladb/scylladb_test.go

+		err = session.Query("SELECT cluster_name FROM system.local").Scan(&cluster_name)
+		require.NoError(t, err)
+
+		require.Equal(t, "Amazing ScyllaDB Test", cluster_name)


suggestion: add awareness about where this constant value comes from

Suggested change

require.Equal(t, "Amazing ScyllaDB Test", cluster_name)

// the "Amazing ScyllaDB Test" cluster name is set in the scylla.yaml file

require.Equal(t, "Amazing ScyllaDB Test", cluster_name)

mdelapenya · 2025-01-10T10:47:49Z

modules/scylladb/scylladb_test.go

+	}, nil
+}
+
+func getDynamoAlternatorClient(t *testing.T, c *scylladb.Container, port int) (*dynamodb.Client, error) {


suggestion: if the assertions in the calling tests are all the same (create a table using the client), you may want to merge them into this helper function, and name it assertCreateTable(t *testing.T). You could also merge the createTable helper

danielhe4rt requested a review from a team as a code owner December 13, 2024 16:29

danielhe4rt and others added 17 commits December 13, 2024 13:33

feat(scylladb): add base packages

672e6a7

feat(scylladb): container implementation with file config and shard-a…

41ed131

…wareness

feat(scylladb): container base test implementation with files and sha…

0003747

…rd-awareness port

feat(scylladb): add makefile test runner command

6a472cb

feat(scylladb): add alternator integration

264d462

feat(scylladb): add WithCustomCommand and code cleanup

680c279

feat(scylladb): implementation cleanup with test coverage

a995a1c

docs(scylladb): add scylladb base documentation module

8f33187

docs: add links to the version it was introduced

7559c61

chore: add scaffolding files

75de222

chore: rename to Container

37ca8fc

chore: run mod tidy

7f7a476

fix: wrong state after code generation

e780e4f

chore: concat strings

95ca13f

chore: use int

01a4fef

chore: calculate variable once

4388885

chore: simplify types

c0f97ce

danielhe4rt force-pushed the main branch from 9d50ec1 to c0f97ce Compare December 13, 2024 16:33

stevenh requested changes Dec 15, 2024

View reviewed changes

eddumelendez reviewed Dec 16, 2024

View reviewed changes

eddumelendez reviewed Dec 17, 2024

View reviewed changes

eddumelendez mentioned this pull request Dec 17, 2024

Add scylladb testcontainers/testcontainers-java#8002

Open

danielhe4rt and others added 2 commits December 18, 2024 13:23

chore(scylladb): WithConfig standardising

82e5352

Co-authored-by: Steven Hartland <[email protected]>

Merge branch 'testcontainers:main' into main

cf070be

danielhe4rt requested a review from mdelapenya January 9, 2025 19:03

mdelapenya reviewed Jan 10, 2025

View reviewed changes

Merge branch 'testcontainers:main' into main

90caba2

chore(scylladb): WithConfig enhancements

aabf4d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scylladb): add scylladb provider #2919

feat(scylladb): add scylladb provider #2919

danielhe4rt commented Dec 13, 2024

netlify bot commented Dec 13, 2024 •

edited

Loading

stevenh left a comment

stevenh Dec 15, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

eddumelendez Dec 16, 2024

danielhe4rt Dec 18, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

stevenh Dec 15, 2024

eddumelendez Dec 16, 2024

eddumelendez Dec 17, 2024 •

edited

Loading

eddumelendez Dec 17, 2024

mdelapenya Jan 7, 2025

mdelapenya Jan 7, 2025

danielhe4rt Jan 8, 2025

mdelapenya left a comment

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

mdelapenya Jan 10, 2025

	func WithAlternator(alternatorPort int) testcontainers.CustomizeRequestOption {
	func WithAlternator(alternatorPort uint16) testcontainers.CustomizeRequestOption {

	func (c Container) ConnectionHost(ctx context.Context, port int) (string, error) {
	func (c Container) ConnectionHost(ctx context.Context, port uint16) (string, error) {

	t.Run("test without shard awareness", func(t *testing.T) {
	t.Run("without-shard-awareness", func(t *testing.T) {

	scylladb.WithConfigFile(filepath.Join("testdata", "scylla.yaml")),
	scylladb.WithConfig(bytes.NewReader(testConfig)),

	You should provide a valid ScyllaDB configuration file when using the function, otherwise the container will fail to
	You should provide a valid ScyllaDB configuration file as an `io.Reader` when using the function, otherwise the container will fail to

	Also In scenarios in which static partitioning is not desired - like mostly-idle cluster without hard latency
	Also in scenarios in which static partitioning is not desired - like mostly-idle cluster without hard latency


		fmt.Println(state.Running)

		connectionHost, err := scyllaContainer.ConnectionHost(ctx, 8080) // Alternator port

	// WithConfig sets the YAML config file to be used for the ScyllaDB container
	// WithConfig sets the YAML config file as an [io.Reader] to be used for the ScyllaDB container

	func TestScyllaWithConfigFile(t *testing.T) {
	func TestScyllaWithConfig(t *testing.T) {

	require.Equal(t, "Amazing ScyllaDB Test", cluster_name)
	// the "Amazing ScyllaDB Test" cluster name is set in the scylla.yaml file
	require.Equal(t, "Amazing ScyllaDB Test", cluster_name)

feat(scylladb): add scylladb provider #2919

Are you sure you want to change the base?

feat(scylladb): add scylladb provider #2919

Conversation

danielhe4rt commented Dec 13, 2024

What does this PR do?

API Example

Why is it important?

How to test this PR

netlify bot commented Dec 13, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh 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

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

Choose a reason for hiding this comment

eddumelendez Dec 17, 2024 • edited Loading

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

mdelapenya 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

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

netlify bot commented Dec 13, 2024 •

edited

Loading

eddumelendez Dec 17, 2024 •

edited

Loading