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

fix(tablets): remove tablets from experimental features #7512

Closed
wants to merge 1 commit into from

Conversation

yarongilor
Copy link
Contributor

@yarongilor yarongilor commented May 30, 2024

fix(tablets): remove tablets and consistent-topology-changes from experimental features

        tablets feature is not experimental anymore and is default
        to be enabled. tablets is dependent on consistent-topology-changes.
        refs: https://github.com/scylladb/scylladb/pull/18898

refs: scylladb/scylladb#18898

i'm not sure this PR is needed and about its dependencies or anything else..
The PR also assumes there's no need of updating scylla yaml with: "enable_tablets": "false"

Testing

  • [ ]

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

…erimental features

	tablets feature is not experimental anymore and is default
	to be enabled. tablets is dependent on consistent-topology-changes.
	refs: scylladb/scylladb#18898
@yarongilor yarongilor changed the title fix(tablets): remove tablets from experimental features fix(tablets): remove tablets and consistent-topology-changes from experimental features May 30, 2024
@@ -136,7 +136,6 @@ scylla_version: ''
test_upgrade_from_installed_3_1_0: false
target_upgrade_version: ''
disable_raft: true
Copy link
Member

Choose a reason for hiding this comment

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

Is this still true? Do we disable raft by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC this flag is not relevant anymore, unless still relevant to only 1 out of 2 raft features. @aleksbykov , @roydahan , can you please confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not.
@aleksbykov / @temichus ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This flag is only was used for rolling upgrade tests. Job with disabled raft uses additional yaml with relevant

if self.params.get("enable_tablets_on_upgrade"):
with node.remote_scylla_yaml() as scylla_yml:
current_experimental_features = scylla_yml.experimental_features
current_experimental_features.remove("tablets")
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead edit scylla.yaml to remove the newly introduced enable_tablets config option?
(See scylladb/scylladb#18898)

@roydahan
Copy link
Contributor

roydahan commented Jun 2, 2024

@yarongilor your PR shouldn't touch raft experimntal - @aleksbykov / @temichus please organise it so whatever should be tested with "raft disabled" (consistent-topology) is correctly configured and remove all the unnecessary flags / configurations.

@yarongilor your PR should flip the flag, now to support "disabling" tablets and have the duplicated jobs that are testing tablets with "disabled raft" - @aleksbykov & @temichus should be synced because some of these should be for 6.0 (so maybe they already working on it).

@yarongilor
Copy link
Contributor Author

yarongilor commented Jun 3, 2024

@yarongilor your PR shouldn't touch raft experimntal - @aleksbykov / @temichus please organise it so whatever should be tested with "raft disabled" (consistent-topology) is correctly configured and remove all the unnecessary flags / configurations.

@yarongilor your PR should flip the flag, now to support "disabling" tablets and have the duplicated jobs that are testing tablets with "disabled raft" - @aleksbykov & @temichus should be synced because some of these should be for 6.0 (so maybe they already working on it).

Conclusions:
This PR will only change tablets flag and will modify a directory to have tablets disabled.
The upgrade will also be adjusted to add the tablets flag.

@roydahan , @ShlomiBalalis , please advise - one thing we have to confirm is the tablets supported nemeses.
Currently, when tablets are enabled by default, the file of "configurations/tablets_supported_nemeses.yaml" is meaningless. All nemeses are now used with tablets in all jobs.
So if it is confirmed, the above file could be removed.
The last configuration is 37 out of 91 nemeses are not supported -

$ grep "\- disrupt_" nemesis.yml -c
91
$ grep "tablets_phase_1_supported = False" nemesis.yml -c
37

cc: @bhalevy

@yarongilor yarongilor changed the title fix(tablets): remove tablets and consistent-topology-changes from experimental features fix(tablets): remove tablets from experimental features Jun 4, 2024
@yarongilor
Copy link
Contributor Author

Addressed in #7295 and #7655

@yarongilor yarongilor closed this Jun 16, 2024
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.

4 participants