Skip to content

Conversation

@dkropachev
Copy link
Contributor

Cassandra 3.0 and Scylla older than 6.x does not support parallel node initialization.
Tune CCM to start nodes one by one when server does not support parallel node initialization.

@dkropachev dkropachev requested a review from fruch April 22, 2025 14:18
@dkropachev dkropachev self-assigned this Apr 22, 2025
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

this logic should be in whom use CCM
those parameters are not really related to parallel execution, they are they for testing purposes to error paths.

I don't think this logic should be in CCM

@dkropachev
Copy link
Contributor Author

this logic should be in whom use CCM those parameters are not really related to parallel execution, they are they for testing purposes to error paths.

I don't think this logic should be in CCM

Added --parallel-start option, if not set, default is used, which is calculated at Cluster.parallel_start_supported

@dkropachev dkropachev force-pushed the dk/fix-cassandra-cluster-start branch from 59c7cae to 7127f24 Compare April 24, 2025 06:45
Cassandra 3.0 and Scylla older than 6.x does not support parallel node initialization.
Tune CCM to start nodes one by one for it.
@dkropachev dkropachev force-pushed the dk/fix-cassandra-cluster-start branch from 7127f24 to b2b9ac9 Compare April 24, 2025 07:06

@property
def parallel_start_supported(self):
return self.database_version.startswith('3.')
Copy link
Contributor

Choose a reason for hiding this comment

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

4.x doesn't support it ?

self.default_wait_for_binary_proto = 420 if self.scylla_mode != 'debug' else 900

@property
def parallel_start_supported(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

the logic about version support, should be in the test/framework trying to enable it.

the versions use here are not even correct, it was supported much before 2025.1.
again the support matrix for thing kind of thing, should be in here.

if jvm_args is None:
jvm_args = []
if parallel_start is None:
parallel_start = self.parallel_start_supported
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be explicit, not like that.

this gonna break all of dtest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why ? we already start scylla nodes in parallel

@fruch
Copy link
Contributor

fruch commented Apr 24, 2025

@dkropachev what is the context of this one, why are you trying to change the default behavior, for what end exactly ?

just cause you don't like the wait_for_* parameter, of for a specific test (or test configuration) you can't do without this change ?

@dkropachev
Copy link
Contributor Author

@dkropachev what is the context of this one, why are you trying to change the default behavior, for what end exactly ?

just cause you don't like the wait_for_* parameter, of for a specific test (or test configuration) you can't do without this change ?

Context is that I have tried to start cassandra cluster with many nodes and couple of times it failed with token ring collission, due the fact that we are starting them in parallel.

@fruch
Copy link
Contributor

fruch commented Apr 24, 2025

@dkropachev what is the context of this one, why are you trying to change the default behavior, for what end exactly ?

just cause you don't like the wait_for_* parameter, of for a specific test (or test configuration) you can't do without this change ?

Context is that I have tried to start cassandra cluster with many nodes and couple of times it failed with token ring collission, due the fact that we are starting them in parallel.

So pass the wait_for parameters and you are fine. Why do you need automatically do that decision?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants