-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
abstract_replication_strategy: prepare options: warn about replication_factor corner cases #16115
base: master
Are you sure you want to change the base?
abstract_replication_strategy: prepare options: warn about replication_factor corner cases #16115
Conversation
…n_factor corner cases Using the replication_factor option with NetworkTopologyStrategy can currently lead to surprising, counter-intuitive results. When creating a keyspace, the option initially sets the replication factor on all DCs. But when used in ALTER KEYSPACE queries, the legacy `replication_factor` option is applied only to DCs added after the keyspace was created, while existing DCs are unaffected, and if no DCs were added, the option is silently ignored. This change detects when the `replication_factor` can be applied safely, and otherwise, it prints a warning. Fixes scylladb#8881 Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Cover a couple more corner cases related to the expansion of the replication_factor option with NetworkTopologyStrategy. Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Cc @ptrsmrn |
auto it = options.find(ks_prop_defs::REPLICATION_FACTOR_KEY); | ||
if (it != options.end()) { | ||
// Expand: the user explicitly provided a 'replication_factor'. | ||
rf = it->second; | ||
options.erase(it); | ||
logger.warn("Using the \"replication_factor\" option is not recommended, but was used for keyspace \"{}\". " | ||
"Use per-datacenter replication factor options instead.", ks_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning in the log is not helpful. It will likely be noticed a long time after the keyspace was used. Better to warn via CQL.
Why is it not recommended across the board? I can understand it in some cases, but here it's too broad.
auto it = options.find(ks_prop_defs::REPLICATION_FACTOR_KEY); | ||
if (it != options.end()) { | ||
// Expand: the user explicitly provided a 'replication_factor'. | ||
rf = it->second; | ||
options.erase(it); | ||
logger.warn("Using the \"replication_factor\" option is not recommended, but was used for keyspace \"{}\". " | ||
"Use per-datacenter replication factor options instead.", ks_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to put a warning in this place. You're suggesting that using "replication_factor" not recommended, Why? It's a feature we added deliberately (and I think @vladzcloudius expressed his enthusiasm with this feature in the last discussion), we called it "auto expand", and it's not un-recommended or deprecated, so why say that it is? If you want to un-recommend it, we need to update the documentation (and discuss it with @tzach and @vladzcloudius ).
// Warn about non-trivial cases | ||
if (new_dc_rfs) { | ||
if (old_dc_rfs) { | ||
logger.warn("The \"replication_factor\": {} option was applied only to newly added datacenters. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I meant when I suggested to warn - this was supposed to be a CQL warning, not a logger warning.
I guess we can have both (in case the user does ALTER KEYSPACE through some sort of automation script and not cqlsh), but I think just the logger, as you did here, is not enough. It doesn't tell the right person of the problem, and it doesn't say it immediately... One person types "ALTER KEYSPACE" in cqlsh, another person notices this warning a few days later.
for (const auto& dc : tm.get_topology().get_datacenters()) { | ||
options.emplace(dc, *rf); | ||
// Warn about non-trivial cases | ||
if (new_dc_rfs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about the need for this logic, and all the different counters and variables and hash tables you added in this code... Isn't it enough to warn if the loop above did "options.insert(opt)" on any option, which means one DC's RF was kept unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I don't see any way to test if this logic is correct without writing a test that runs various cases and checks for the warnings. You can also do it manually in cqlsh and check the behavior, but then we don't have a regression test (and I as a reviewer don't know what you tested).
logger.warn("\"replication_factor\": {} was inherited from a previous replication strategy, where it may have been applied globally in the cluster. " | ||
"It now applies to the following topology: {}. Current replication options are: {}. " | ||
"Run repair to rebuild the new token replicas and then cleanup to remove the stale replicas", | ||
*rf, dc_racks, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Unless I'm misunderstanding something, this warning wasn't part of #8881 or mentioned in the commit message, but a new kind of safety check you are proposing here.
But I think (please correct me if I'm wrong) that this check is not only relevant when you have a global rf - even if you specify a separate RF per data center, when you set RF in a data center which didn't have one, you need to know to do a repair. I think this message should happen in any such case, not only when replication_factor is used.
Anyway, this check probably belongs in a separate patch with its own justification.
I find the explanation of how this feature works way more clear than how it is described in our documentation - https://opensource.docs.scylladb.com/stable/cql/ddl.html#networktopologystrategy - changing the docs would be helpful for future readers. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
In the past, there was opposition to this from @vladzcloudius. I don't remember the arguments, just that there were some. |
opposition to what exactly? |
Sorry. IIRC the opposition was against a PR which wanted to forbid ambiguous changes to keyspace replication. So it doesn't apply here. Personally, I think a CQL warning is the way to go. |
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See #8881, #15391, #16115 (cherry picked from commit b875151)
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See scylladb#8881, scylladb#15391, scylladb#16115 (cherry picked from commit b875151)
This patch removes the support for the "wildcard" replication_factor option for ALTER KEYSPACE when the keyspace supports tablets. It will still be supported for CREATE KEYSPACE so that a user doesn't have to know all datacenter names when creating the keyspace, but ALTER KEYSPACE will require that and the user will have to specify the exact change in replication factors they wish to make by explicitly specifying the datacenter names. Expanding the replication_factor option in the ALTER case is unintuitive and it's a trap many users fell into. See #8881, #15391, #16115
Using the replication_factor option with
NetworkTopologyStrategy can currently lead to surprising, counter-intuitive results.
When creating a keyspace, the option initially sets the replication factor on all DCs.
But when used in ALTER KEYSPACE queries, the legacy
replication_factor
option is applied only toDCs added after the keyspace was created, while
existing DCs are unaffected, and if no DCs were
added, the option is silently ignored.
This change detects when the
replication_factor
can be applied safely, and otherwise, it prints a warning.
Fixes #8881