Fixes for handling SCYLLA_EXT_OPTS correctly
#541
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
December 10, 2023 10:58
when starting a cluster with an `SCYLLA_EXT_OPTS` env variable, if the user sets the smp or memory using the abbreviated option name (-c or -m), ScyllaDB will fail on startup due to double use of parameters. This is because scylla-ccm fails to account for --smp and -c (for example) as the same option, injecting the literal option into the command line parameters. Here we change this for --smp and --memory, with the option to add more similar cases in the future. Fixes scylladb#539 Signed-off-by: Eliran Sinvani <[email protected]>
Some scylla command lines options may and might appear more than once with different values, a concrete example for this, which this commit handles is `--experimental-features` when more than one experimental feature needs to be enabled. Fixes scylladb#540 Signed-off-by: Eliran Sinvani <[email protected]>
|
I will review this change once I am back from my PTO. |
|
@tchaikov ping |
tchaikov
reviewed
Jan 15, 2024
ccmlib/scylla_node.py
Outdated
| @@ -570,11 +570,12 @@ def process_opts(opts): | |||
| opts_i += 1 | |||
| val = ' '.join(vals) | |||
| if key not in ext_args and not key.startswith("--scylla-manager"): | |||
| ext_args[key] = val | |||
| ext_args[args_translation_map.get(key, key)] = val | |||
There was a problem hiding this comment.
i'd suggest normalize key even before the if key not in ext_args and not key.startswith("--scylla-manager") check, and do this unconditially. like:
key = args_translation_map.get(key, key)
if key not in ext_args and not key.startswith("--scylla-manager"):
ext_args[key] = valfor better readaiblity.
tchaikov
reviewed
Jan 15, 2024
| ext_args = OrderedDict() | ||
| def should_add_option(key): |
There was a problem hiding this comment.
the nested function is more readable, but it also renders this method more complicated, how about this?
if key.startswith("--scylla-manager"):
continue
if key in allowed_duplicates_set:
ext_args.get(key, []).append(val)
elif key not in ext_args:
ext_args[key] = valfor following reasons:
- the combinations of conditions is getting convoluted in this function, better off simplifying them
dict.set_default()helps to consolidate the logic, but it hurts the readability.- we should not use list for all values of options. the boxing-unboxing hurts the readability.
|
@eliransin hi Eliran, sorry for the latency. left a couple comments. |
This was referenced Jan 29, 2024
fruch
added a commit
to fruch/scylla-ccm
that referenced
this pull request
Feb 5, 2024
since we have cases multiple `--experimental-features` needs to be passed via `SCYLLA_EXT_OPTS`, adding support for key with multiple values in `process_opts()` fixes: scylladb#540 closes: scylladb#541
fruch
added a commit
to fruch/scylla-ccm
that referenced
this pull request
Feb 6, 2024
since we have cases multiple `--experimental-features` needs to be passed via `SCYLLA_EXT_OPTS`, adding support for key with multiple values in `process_opts()` fixes: scylladb#540 closes: scylladb#541
fruch
added a commit
to fruch/scylla-ccm
that referenced
this pull request
Feb 6, 2024
since we have cases multiple `--experimental-features` needs to be passed via `SCYLLA_EXT_OPTS`, adding support for key with multiple values in `process_opts()` fixes: scylladb#540 closes: scylladb#541
|
closing in favor of #558 |
fruch
added a commit
to fruch/scylla-ccm
that referenced
this pull request
Mar 10, 2024
since we have cases multiple `--experimental-features` needs to be passed via `SCYLLA_EXT_OPTS`, adding support for key with multiple values in `process_opts()` fixes: scylladb#540 closes: scylladb#541
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This mini-series implements two fixes for handling
SCYLLA_EXT_OPTS:-cand-moptions which are abbreviations for the--smpand--memoryoptions and handles them correctly.--experimental-featureswith correct handling.The individual issues:
#539 and #540 describes the specific problems.