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

Move Cassandra env variable setup to protected method #5991

Closed
wants to merge 1 commit into from
Closed

Move Cassandra env variable setup to protected method #5991

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions modules/cassandra/src/main/java/org/testcontainers/containers/CassandraContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@ public CassandraContainer(DockerImageName dockerImageName) {

addExposedPort(CQL_PORT);
this.enableJmxReporting = false;

withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch");
withEnv("JVM_OPTS", "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0");
withEnv("HEAP_NEWSIZE", "128M");
withEnv("MAX_HEAP_SIZE", "1024M");
withEnv("CASSANDRA_ENDPOINT_SNITCH", "GossipingPropertyFileSnitch");
withEnv("CASSANDRA_DC", DEFAULT_LOCAL_DATACENTER);
Comment on lines -72 to -77
Copy link
Member

Choose a reason for hiding this comment

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

IIRC those env were added in order to improve cassandra startup. I think we can apply those only if the image name is cassandra. @kiview WDYT?

It can be also applied for library/cassandra for those using a local registry. but we can do something like new CassandraContainer("library/cassandra:3.2.1").applyDefaultEnvVars(). Just throwing an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yeah no strong opinions on the mechanism apart from maybe not coupling it to the image name directly, seems a little brittle and might still cause problems for anyone who copies their own cassandra.yaml in into the container. Moving it to an overridable protected method was the least obtrusive way to allow turning it off but making it explicitly opt-in is fine too! I suppose we'll wait for Kevin to weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

@eddumelendez I think our container implementations should always assume a compatible image and not bind specific integration to the name. Else, it will become more of a hassle, once users provide their own image (also in this case we should assume a working image as default IMO).

@akhaku TBH, I don't understand the PR fully. Isn't it enough, if you do setEnv(new ArrayList() if you want to remove the defaults here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for that, I didn't realize that I could empty out the environment map like that. That workaround is fine by me!
Moving it to a protected method makes it easer to extend (e.g. I could call super.initEnv in my overridden method before or after adding my own env vars) but calling setEnv with an empty list works for my use case! I'll go ahead and decline this PR for now.

Also, only just realized that I forgot to add a call to initEnv in the constructor, I meant to have that there so it doesn't change existing behavior 🤦 sorry that probably caused some confusion too.

}

@Override
Expand Down Expand Up @@ -131,6 +124,19 @@ protected void optionallyMapResourceParameterAsVolume(String pathNameInContainer
.ifPresent(mountableFile -> withCopyFileToContainer(mountableFile, pathNameInContainer));
}

/**
* Set up environment variables to allow Cassandra to start up. You may choose to override this method for example
* if you want to set your own variables to run multi-node clusters.
*/
protected void initEnv() {
withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch");
withEnv("JVM_OPTS", "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0");
withEnv("HEAP_NEWSIZE", "128M");
withEnv("MAX_HEAP_SIZE", "1024M");
withEnv("CASSANDRA_ENDPOINT_SNITCH", "GossipingPropertyFileSnitch");
withEnv("CASSANDRA_DC", getLocalDatacenter());
}

/**
* Initialize Cassandra with the custom overridden Cassandra configuration
* <p>
Expand Down