[
https://issues.apache.org/jira/browse/CASSANDRA-15254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17686813#comment-17686813
]
Maxim Muzafarov edited comment on CASSANDRA-15254 at 2/10/23 12:45 AM:
-----------------------------------------------------------------------
[~dcapwell],
We need to agree on a clear rule of "how to check whether a property is
modifiable at runtime or not" before we can proceed to change properties via
the SettingsTable. This is a crucial point for the design.
I would say that relying on the _volatile_ keyword to distinguish which
configuration property is mutable is a damned bad idea, you even mentioned
yourself that it's not 100% correct. And here are the drawbacks that make this
idea even worse:
# You do not control what exactly you expose to a user. If you're modifying
the JMX API, you're writing a new method for the MBean class to explicitly
expose a new property to a user, which in turn makes the exposition clear. The
same is not applicable for a virtual table and even doesn't exist for the
SettingsTable. Why? We have to provide it in this solution.
# The volatile keyword is not the only way to achieve thread safety, so just
because we don't see it doesn't mean the usage is unsafe and can't be modified.
# Take a look at the following example, the field has the volatile keyword,
but it must not be exposed to the public.
{code:java}
@VisibleForTesting
public static void setEnableDropCompactStorage(boolean enableDropCompactStorage)
{
// drop_compact_storage_enabled is volatile.
conf.drop_compact_storage_enabled = enableDropCompactStorage;
}{code}
Actually, I did the same exercise to find out how many mismatches in method and
property names we have with the following criteria:
* Looking through the DatabaseDescriptor and GuardrailsOptions, they both make
configuration changes;
* I count not only that method with camel case and field names match but
method and field types as well;
* I also enriched my previous results with an additional search for properties
with the volatile keyword and a search for the "right" JMX methods through the
whole set of MBean classes;
* Configuration fields with the Deprecated annotation were excluded to be more
accurate;
My results of a comparison of a field name with the corresponding methods name
in camel case:
{code:bash}
total: 345
setters: 109, missed 236
volatile setters: 90, missed 255
jmx setters: 35, missed 310
getters: 139, missed 206
volatile getters: 107, missed 238
jmx getters: 63, missed 282 {code}
The link to the source code:
[https://github.com/Mmuzaf/cassandra/blob/cassandra-15254-wip/src/java/org/apache/cassandra/utils/AccessorFields.java#L61]
I see now we have a good starting point for making the configuration properties
associated with the 35 JMX setter methods that fully match the property name to
make them available for modification by SettingsTable for the first step,
leaving the others for the next.
So, in the end, we will have:
* A clear exposition rule for configuration property - if a property name and
its type match the corresponding method in the DatabaseDescriptor
(GuardrailsOptions), this means that the property can be modified by public
APIs;
* We will end up with consistency through APIs (JMX and SettingsTable) by
implementing everything step by step;
* If a property name needs to be renamed, the auto-generated ConfigFields will
immediately show which other calls we should fix;
* We will set new property values by calling the right setter methods in the
DatabaseDescriptor (GuardrailsOptions), which will also perform all the
necessary input value checks as it does now. We can rely on @Replaces to
support backward compatibility for old names;
Thoughts?
was (Author: mmuzaf):
[~dcapwell],
We need to agree on a clear rule of "how to check whether a property is
modifiable at runtime or not" before we can proceed to change properties via
the SettingsTable. This is a crucial point for the design.
I would say that relying on the _volatile_ keyword to distinguish which
configuration property is mutable is a damned bad idea, you even mentioned
yourself that it's not 100% correct. And here are the drawbacks that make this
idea even worse:
# You do not control what exactly you expose to a user. If you're modifying
the JMX API, you're writing a new method for the MBean class to explicitly
expose a new property to a user, which in turn makes the exposition clear. The
same is not applicable for a virtual table and even doesn't exist for the
SettingsTable. Why? We have to provide it in this solution.
# The volatile keyword is not the only way to achieve thread safety, so just
because we don't see it doesn't mean the usage is unsafe.
# Take a look at the following example, the field has the volatile keyword,
but it must not be exposed to the public.
{code:java}
@VisibleForTesting
public static void setEnableDropCompactStorage(boolean enableDropCompactStorage)
{
// drop_compact_storage_enabled is volatile.
conf.drop_compact_storage_enabled = enableDropCompactStorage;
}{code}
Actually, I did the same exercise to find out how many mismatches in method and
property names we have with the following criteria:
* Looking through the DatabaseDescriptor and GuardrailsOptions, they both make
configuration changes;
* I count not only that method with camel case and field names match but
method and field types as well;
* I also enriched my previous results with an additional search for properties
with the volatile keyword and a search for the "right" JMX methods through the
whole set of MBean classes;
* Configuration fields with the Deprecated annotation were excluded to be more
accurate;
My results of a comparison of a field name with the corresponding methods name
in camel case:
{code:bash}
total: 345
setters: 109, missed 236
volatile setters: 90, missed 255
jmx setters: 35, missed 310
getters: 139, missed 206
volatile getters: 107, missed 238
jmx getters: 63, missed 282 {code}
The link to the source code:
[https://github.com/Mmuzaf/cassandra/blob/cassandra-15254-wip/src/java/org/apache/cassandra/utils/AccessorFields.java#L61]
I see now we have a good starting point for making the configuration properties
associated with the 35 JMX setter methods that fully match the property name to
make them available for modification by SettingsTable for the first step,
leaving the others for the next.
So, in the end, we will have:
* A clear exposition rule for configuration property - if a property name and
its type match the corresponding method in the DatabaseDescriptor
(GuardrailsOptions), this means that the property can be modified by public
APIs;
* We will end up with consistency through APIs (JMX and SettingsTable) by
implementing everything step by step;
* If a property name needs to be renamed, the auto-generated ConfigFields will
immediately show which other calls we should fix;
* We will set new property values by calling the right setter methods in the
DatabaseDescriptor (GuardrailsOptions), which will also perform all the
necessary input value checks as it does now. We can rely on @Replaces to
support backward compatibility for old names;
Thoughts?
> Allow UPDATE on settings virtual table to change running configurations
> -----------------------------------------------------------------------
>
> Key: CASSANDRA-15254
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15254
> Project: Cassandra
> Issue Type: New Feature
> Components: Feature/Virtual Tables
> Reporter: Chris Lohfink
> Assignee: Maxim Muzafarov
> Priority: Normal
> Attachments: Configuration Registry Diagram.png
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Allow using UPDATE on the system_views.settings virtual table to update
> configs at runtime for the equivalent of the dispersed JMX
> attributes/operations.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
