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

db: config: introduce force-gossip-topology-changes #18284

Closed
wants to merge 1 commit into from
Closed

db: config: introduce force-gossip-topology-changes #18284

wants to merge 1 commit into from

Conversation

patjed41
Copy link
Contributor

@patjed41 patjed41 commented Apr 17, 2024 •
edited
Loading

We are going to make the consistent-topology-changes experimental
feature unused in 6.0. However, the topology upgrade procedure will
be manual and voluntary, so some 6.0 clusters will be using the
gossip-based topology. Therefore, we need to continue testing the
gossip-based topology. The solution is introducing a new flag,
force-gossip-topology-changes, that will enforce the gossip-based
topology in a fresh cluster.

In this patch, we only introduce the parameter without any effect.
Here is the explanation. Making consistent-topology-changes unused
and introducing force-gossip-topology-changes requires adjustments
in scylla-dtest. We want to merge changes to scylladb and scylla-dtest
in a way that ensures all tests are run correctly during the whole
process. If we merged all changes to scylladb first, before merging
the scylla-dtest changes, all tests would run with the raft-based
topology and the ones excluded in the raft-based topology would fail.
We also can't merge all changes to scylla-dtest first. However, we
can follow this plan:

  1. scylladb: merge this patch
  2. scylla-dtest: start using force-gossip-topology-changes
    in jobs that run without the raft-based topology
  3. scylladb: merge the rest of the changes
  4. scylla-dtest: merge the rest of the changes

Ref #17802

@patjed41 patjed41 added the backport/none Backport is not required label Apr 17, 2024
@patjed41 patjed41 mentioned this pull request Apr 17, 2024
Merged
@scylladb-promoter
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
✅ - dtest with topology changes
❌ - dtest
✅ - Unit Tests

Failed Tests (1/30308):

Build Details:

  • Duration: 2 hr 23 min
  • Builder: spider1.cloudius-systems.com

@avikivity
Copy link
Member

Please avoid negative options. They require extra effort to evaluate a double negative.

@kbr-scylla
Copy link
Contributor

@avikivity in this case it's intended.

  • it implies that you need to take action to disable the feature
  • also, the option isn't supposed to be used by users/customers. We don't want to advertise that it exists. It's supposed to be used only for testing

@kbr-scylla
Copy link
Contributor

🔴 CI State: FAILURE

#18278

@avikivity
Copy link
Member

@avikivity in this case it's intended.

  • it implies that you need to take action to disable the feature

It's still possible to do it without double negatives. --consistent-topology=false (default true).

  • also, the option isn't supposed to be used by users/customers. We don't want to advertise that it exists. It's supposed to be used only for testing

We should treat ourselves well too.

@patjed41
Copy link
Contributor Author

patjed41 commented Apr 18, 2024 •
edited
Loading

@avikivity in this case it's intended.

  • it implies that you need to take action to disable the feature

It's still possible to do it without double negatives. --consistent-topology=false (default true).

I agree that we should get rid of double negatives, but I don't think consistent-topology is a good idea. The name should:

  • be significantly different from consistent-topology-changes so anyone who sees it notices the difference,
  • warn of changing the default value as this flag is supposed to be used only for testing.

What about force-gossip-topology-changes (default false)?

I'd like to confirm the final name before making any changes as they will take a lot of work (multiple PRs in multiple repos).

@kbr-scylla
Copy link
Contributor

What about force-gossip-topology-changes (default false)?

I like it.

@avikivity @tzach do you accept?

@avikivity
Copy link
Member

What about force-gossip-topology-changes (default false)?

I like it.

@avikivity @tzach do you accept?

I like it too

@patjed41 patjed41 mentioned this pull request Apr 18, 2024
Closed
@patjed41 patjed41 force-pushed the introduce-disable-consistent-topology-changes branch from 0edc725 to 0692652 Compare April 22, 2024 14:48
@patjed41
Copy link
Contributor Author

rebased

@patjed41 patjed41 force-pushed the introduce-disable-consistent-topology-changes branch from 0692652 to 003cd8b Compare April 22, 2024 14:50
@patjed41
db: config: introduce force-gossip-topology-changes
80254e4 
We are going to make the `consistent-topology-changes` experimental
feature unused in 6.0. However, the topology upgrade procedure will
be manual and voluntary, so some 6.0 clusters will be using the
gossip-based topology. Therefore, we need to continue testing the
gossip-based topology. The solution is introducing a new flag,
`force-gossip-topology-changes`, that will enforce the gossip-based
topology in a fresh cluster.

In this patch, we only introduce the parameter without any effect.
Here is the explanation. Making `consistent-topology-changes` unused
and introducing `force-gossip-topology-changes` requires adjustments
in scylla-dtest. We want to merge changes to scylladb and scylla-dtest
in a way that ensures all tests are run correctly during the whole
process. If we merged all changes to scylladb first, before merging
the scylla-dtest changes, all tests would run with the raft-based
topology and the ones excluded in the raft-based topology would fail.
We also can't merge all changes to scylla-dtest first. However, we
can follow this plan:
1. scylladb: merge this patch
2. scylla-dtest: start using `force-gossip-topology-changes`
   in jobs that run without the raft-based topology
3. scylladb: merge the rest of the changes
4. scylla-dtest: merge the rest of the changes

Ref scylladb#17802
@patjed41 patjed41 force-pushed the introduce-disable-consistent-topology-changes branch from 003cd8b to 80254e4 Compare April 22, 2024 14:56
@patjed41
Copy link
Contributor Author

patjed41 commented Apr 22, 2024 •
edited
Loading

v2:

  • renamed disable-consistent-topology-changes to force-gossip-topology-changes
  • updated the option's description so it better suits its new name
  • made a small change in the commit message: "remove consistent-topology-changes" -> "make consistent-topology-changes unused" - we don't really remove the feature, we only remove any effect it might have on the code
  • updated the PR title and removed "unused" from it because it was confusing - "unused" has a different meaning when describing a flag

@patjed41 patjed41 changed the title db: config: introduce unused disable-consistent-topology-changes db: config: introduce force-gossip-topology-changes Apr 22, 2024
@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest
✅ - dtest with topology changes
✅ - Unit Tests

Build Details:

  • Duration: 5 hr 41 min
  • Builder: i-0776277565fab83bb (m5ad.8xlarge)

kbr-scylla added a commit that referenced this pull request Apr 26, 2024
@kbr-scylla
Merge 'db: config: move consistent-topology-changes out of experiment…
d8313dd 
…al and make it the default for new clusters' from Patryk Jędrzejczak

We move consistent cluster management out of experimental and
make it the default for new clusters in 6.0. In code, we make the
`consistent-topology-changes` flag unused and assumed to be true.

In 6.0, the topology upgrade procedure will be manual and
voluntary, so some clusters will still be using the gossip-based
topology even though they support the raft-based topology.
Therefore, we need to continue testing the gossip-based topology.
This is possible by using the `force-gossip-topology-changes` flag
introduced in #18284.

Ref #17802

Closes #18285

* github.com:scylladb/scylladb:
  docs: raft.rst: update after removing consistent-topology-changes
  treewide: fix indentation after the previous patch
  db: config: make consistent-topology-changes unused
  test: lib: single_node_cql_env: restart a node in noninitial run_in_thread calls
  test: test_read_required_hosts: run with force-gossip-topology-changes
  storage_service: join_cluster: replace force_gossip_based_join with force-gossip-topology-changes
  storage_service: join_token_ring: fix finish_setup_after_join calls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbr-scylla kbr-scylla kbr-scylla approved these changes

Assignees
No one assigned
Labels
backport/none Backport is not required promoted-to-master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@patjed41 @scylladb-promoter @avikivity @kbr-scylla