Skip to content

Replace SimpleStrategy with NetworkTopologyStrategy in integration tests#830

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

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

Conversation

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator

@sylwiaszunejko sylwiaszunejko commented Apr 22, 2026

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.

@sylwiaszunejko sylwiaszunejko self-assigned this Apr 22, 2026
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch from 36291ce to bb73387 Compare April 22, 2026 09:41
dkropachev
dkropachev reviewed Apr 22, 2026
View reviewed changes
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
dkropachev
dkropachev requested changes Apr 22, 2026
View reviewed changes
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

sylwiaszunejko commented Apr 22, 2026

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

Lorak-mmk 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.

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator Author

sylwiaszunejko commented Apr 23, 2026 •
edited
Loading

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

dkropachev
dkropachev requested changes Apr 23, 2026
View reviewed changes
Comment thread tests/integration/__init__.py Outdated
Comment thread tests/integration/__init__.py Outdated
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch from be1c0c9 to 25d7927 Compare April 23, 2026 15:56
@sylwiaszunejko
Copy link
Copy Markdown
Collaborator Author

sylwiaszunejko commented May 4, 2026

@dkropachev @Lorak-mmk Please rereview

Lorak-mmk
Lorak-mmk requested changes May 4, 2026
View reviewed changes
Comment thread tests/integration/standard/test_cython_protocol_handlers.py
Comment thread tests/integration/standard/test_metadata.py
Comment thread tests/integration/standard/test_metadata.py
Comment thread tests/integration/standard/test_metadata.py
Comment thread tests/integration/__init__.py Outdated
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch 2 times, most recently from 1472447 to 0e1b4a7 Compare May 4, 2026 15:02
@sylwiaszunejko
tests: replace SimpleStrategy with NetworkTopologyStrategy
f6ae326 
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.
@sylwiaszunejko
tests: bootstrap 3 new nodes in full node replacement test
248d659 
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.
@sylwiaszunejko
tests: xfail LWT tests on Scylla versions without tablet LWT support
4bfadcb 
LWT is not supported with tablets on ScyllaDB < 2025.4. Mark the affected SerialConsistencyTests and
LightweightTransactionTests as xfail for those versions.
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch 2 times, most recently from e98a599 to 2a1029d Compare May 5, 2026 09:17
@sylwiaszunejko
tests: xfail tests on Scylla version without indexes tablet support
0f8a61d 
Secondary indexes are not supported on base tables with tablets for Scylla
versions < 2026.1.
@sylwiaszunejko
test_replicas_are_queried: use dedicated keyspace with RF=1 and table…
f4097da 
…ts disabled
@sylwiaszunejko sylwiaszunejko force-pushed the remove-simple-strategy branch from 2a1029d to f4097da Compare May 5, 2026 09:52
@sylwiaszunejko sylwiaszunejko requested a review from Lorak-mmk May 5, 2026 10:30
@Lorak-mmk
Copy link
Copy Markdown

Lorak-mmk commented May 5, 2026

@sylwiaszunejko If you addressed some of my earlier comments, could you mark them as resolved? And if you decided to not address some of them, could you respond with the reason?

@sylwiaszunejko
Copy link
Copy Markdown
Collaborator Author

sylwiaszunejko commented May 5, 2026

@sylwiaszunejko If you addressed some of my earlier comments, could you mark them as resolved? And if you decided to not address some of them, could you respond with the reason?

All resolved

Lorak-mmk
Lorak-mmk approved these changes May 5, 2026
View reviewed changes
Comment thread tests/integration/cqlengine/query/test_named.py
Comment on lines 293 to 297

@greaterthanorequalcass30
@xfail_scylla_version_lt(reason='scylladb/scylladb#22677 - Materialized views and secondary indexes are not supported on base tables with tablets.',
oss_scylla_version='7.0', ent_scylla_version='2026.1')
@execute_count(5)
Copy link
Copy Markdown

@Lorak-mmk Lorak-mmk May 5, 2026

Choose a reason for hiding this comment

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

Unrelated: the oss_scylla_version parameter no longer makes sense. In a separate PR we should modify xfail_scylla_version_lt so that it takes only one scylla_version parameter.

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

Reviewers

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

@dkropachev dkropachev Awaiting requested review from dkropachev

Requested changes must be addressed to merge this pull request.

Assignees

@sylwiaszunejko sylwiaszunejko

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sylwiaszunejko @Lorak-mmk @dkropachev