Skip to content

Replace SimpleStrategy with NetworkTopologyStrategy in integration tests#830

Open
sylwiaszunejko wants to merge 3 commits intoscylladb:masterfrom
sylwiaszunejko:remove-simple-strategy
Open

Replace SimpleStrategy with NetworkTopologyStrategy in integration tests#830
sylwiaszunejko wants to merge 3 commits intoscylladb:masterfrom
sylwiaszunejko:remove-simple-strategy

Conversation

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator

In tests/integration/init.py, SimpleStrategy is kept but tablets are explicitly disabled to avoid decommission failures.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Comment thread tests/integration/__init__.py Outdated
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch 3 times, most recently from e35fa7e to 20b5f60 Compare April 22, 2026 13:47
Copy link
Copy Markdown
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

I still see lot's of SimpleStrategy in the test code, is it intended to be that way ?

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator Author

I still see lot's of SimpleStrategy in the test code, is it intended to be that way ?

For places where we won't tablets (like some places with test_metadata) disable I left SimpleStrategy, I can change it to NTS with tablets disabled if that is preferred.
If you mean unit tests, examples or tests that we don't currently run (e.g. in tests/integration/long) I didn't touch them in this PR

@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch 2 times, most recently from 19f2c23 to be1c0c9 Compare April 23, 2026 06:42
@Lorak-mmk
Copy link
Copy Markdown

I can change it to NTS with tablets disabled if that is preferred.

We should use NTS and avoid SS wherever possible.

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator Author

sylwiaszunejko commented Apr 23, 2026

I can change it to NTS with tablets disabled if that is preferred.

We should use NTS and avoid SS wherever possible.

I changed that

Comment thread tests/integration/__init__.py Outdated
ddl = '''
CREATE KEYSPACE test1rf
WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}'''
WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'} AND tablets = {'enabled': false}'''
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still there

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@classmethod
def create_keyspace(cls, rf):
ddl = "CREATE KEYSPACE {0} WITH replication = {{'class': 'SimpleStrategy', 'replication_factor': '{1}'}}".format(cls.ks_name, rf)
ddl = "CREATE KEYSPACE {0} WITH replication = {{'class': 'SimpleStrategy', 'replication_factor': '{1}'}} AND tablets = {{'enabled': false}}".format(cls.ks_name, rf)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one too

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it's BasicKeyspaceUnitTestCase so unit test, I only changed code related to integration tests in this PR

Replace SimpleStrategy with NetworkTopologyStrategy across integration
tests to align with ScyllaDB's tablet-based replication defaults.

In the tablets test module, skip default keyspace creation
(set_keyspace=False) to avoid RF=3 keyspaces that block node
decommission when all nodes already hold replicas.
With tablets enabled, decommissioning a node from a 3-node cluster with
RF=3 fails because there is no available node to receive tablet replicas.
Bootstrap 3 replacement nodes instead of 2 so that each original node
can be decommissioned while sufficient replicas remain.
LWT is not supported with tablets on ScyllaDB < 2025.4. Mark the affected SerialConsistencyTests and
LightweightTransactionTests as xfail for those versions.
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.

3 participants