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.

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

Add scylladb #8002

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add scylladb #8002

wants to merge 7 commits into from

Conversation

mkorolyov
Copy link

ScyllaDB is an open-source distributed NoSQL wide-column data store. It gains wide popularity as a drop-in replacement for Cassandra but a significantly faster one.

This PR adds the ScyllaDB module.

@mkorolyov mkorolyov requested a review from a team as a code owner December 26, 2023 10:32
@guy9
Copy link

guy9 commented Jan 7, 2024

Thanks @mkorolyov , interested in this as well

@mykaul
Copy link

mykaul commented Jan 14, 2024

I wish there was a way NOT to provide your own scylla.yaml, but merge your defaults with the default scylla.yaml - the end result is that you might be missing some new defaults if you bring your own scylla.yaml.

@mkorolyov
Copy link
Author

I wish there was a way NOT to provide your own scylla.yaml, but merge your defaults with the default scylla.yaml - the end result is that you might be missing some new defaults if you bring your own scylla.yaml.

This looks like a separate feature for merging config files. Usually what I used to see in a majority of the applications is that you are using the default config file and overriding it with ENVs or flags or providing your own config file. Providing own config file which will still be merged with some default looks like implicit behavior which in my opinion will confuse users. WDYT?

@guy9
Copy link

guy9 commented Jan 21, 2024

Any updates on merging this?

@mykaul
Copy link

mykaul commented Jan 23, 2024

I wish there was a way NOT to provide your own scylla.yaml, but merge your defaults with the default scylla.yaml - the end result is that you might be missing some new defaults if you bring your own scylla.yaml.

This looks like a separate feature for merging config files. Usually what I used to see in a majority of the applications is that you are using the default config file and overriding it with ENVs or flags or providing your own config file. Providing own config file which will still be merged with some default looks like implicit behavior which in my opinion will confuse users. WDYT?

Yes, my ask is exactly this - let's use the default Scylla YAML that comes with Scylla, and allow overriding values in it.

this.enableJmxReporting = false;

withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch");
withEnv("JVM_OPTS", "-Dcassandra.skip_wait_for_gossip_to_settle=0 -Dcassandra.initial_token=0");
Copy link

Choose a reason for hiding this comment

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

those are not relevant to scylla, you need commandline option for skip_wait_for_gossip_to_settle

addExposedPort(CQL_PORT);
this.enableJmxReporting = false;

withEnv("CASSANDRA_SNITCH", "GossipingPropertyFileSnitch");
Copy link

Choose a reason for hiding this comment

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

I think this needs to be checked, scylla docker doesn't use this variable

@vbazhmin
Copy link

vbazhmin commented Jun 7, 2024

Hello folks. Any updates on this one ? ;)

@eddumelendez
Copy link
Member

Hi, I'll review the PR once the other comments have been addressed. It is great seeing Scylla team reviewing the PR.

@fruch
Copy link

fruch commented Aug 1, 2024

Hi, I'll review the PR once the other comments have been addressed. It is great seeing Scylla team reviewing the PR.

hopefully it won't be dragged like it's counterpart:
testcontainers/testcontainers-python#167

@eddumelendez
Copy link
Member

@fruch LMK once this is ready to review and will do it as soon as possible. I still see some comments that hasn't been resolved. So, that's the reason why I haven't reviewed yet.

@eddumelendez
Copy link
Member

Hi, I know this is still in progress because I see some comments unresolved. But, JFTR it would be nice to add an integration test enabling ssl and add it to the docs.

@eddumelendez
Copy link
Member

I'm updating the PR based on testcontainers/testcontainers-go#2919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants