Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

;latest GA" on a cadence that provides new improvements and 
> > > functionality to users soon enough to be valuable and relevant.
> > >
> > >
> > > So in this case, target whatever unreleased next feature release (i.e. 
> > > SEMVER MAJOR || MINOR) we have on deck.
> > >
> > > On Thu, Jul 6, 2023, at 1:21 PM, Ekaterina Dimitrova wrote:
> > >
> > > Hi,
> > >
> > > First of all, thank you for all the work!
> > > I personally think that it should be ok to add a new column.
> > >
> > > I will be very happy to see this landing in 5.0.
> > > I am personally against porting this patch to 4.1. To be clear, I am sure 
> > > you did a great job and my response would be the same to every single 
> > > person - the configuration is quite wide-spread and the devil is in the 
> > > details. I do not see a good reason for exception here except 
> > > convenience. There is no feature flag for these changes too, right?
> > >
> > > Best regards,
> > > Ekaterina
> > >
> > > На четвъртък, 6 юли 2023 г. Miklosovic, Stefan 
> > >  написа:
> > >
> > > Hi Maxim,
> > >
> > > I went through the PR and added my comments. I think David also reviewed 
> > > it. All points you mentioned make sense to me but I humbly think it is 
> > > necessary to have at least one additional pair of eyes on this as the 
> > > patch is relatively impactful.
> > >
> > > I would like to see additional column in system_views.settings of name 
> > > "mutable" and of type "boolean" to see what field I am actually allowed 
> > > to update as an operator.
> > >
> > > It seems to me you agree with the introduction of this column (1) but 
> > > there is no clear agreement where we actually want to put it. You want 
> > > this whole feature to be committed to 4.1 branch as well which is an 
> > > interesting proposal. I was thinking that this work will go to 5.0 only. 
> > > I am not completely sure it is necessary to backport this feature but 
> > > your argumentation here (2) is worth to discuss further.
> > >
> > > If we introduce this change to 4.1, that field would not be there but in 
> > > 5.0 it would. So that way we will not introduce any new column to 
> > > system_views.settings.
> > > We could also go with the introduction of this column to 4.1 if people 
> > > are ok with that.
> > >
> > > For the simplicity, I am slightly leaning towards introducing this 
> > > feature to 5.0 only.
> > >
> > > (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
> > > (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
> > >
> > > 
> > > From: Maxim Muzafarov 
> > > Sent: Friday, June 23, 2023 13:50
> > > To: dev@cassandra.apache.org
> > > Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
> > > running configuration
> > >
> > > NetApp Security WARNING: This is an external email. Do not click links or 
> > > open attachments unless you recognize the sender and know the content is 
> > > safe.
> > >
> > >
> > >
> > >
> > > Hello everyone,
> > >
> > >
> > > As there is a lack of feedback for an option to go on with and having
> > > a discussion for pros and cons for each option I tend to agree with
> > > the vision of this problem proposed by David :-) After a lot of
> > > discussion on Slack, we came to the @ValidatedBy annotation which
> > > points to a validation method of a property and this will address all
> > > our concerns and issues with validation.
> > >
> > > I'd like to raise the visibility of these changes and try to find one
> > > more committer to look at them:
> > > https://issues.apache.org/jira/browse/CASSANDRA-15254
> > > https://github.com/apache/cassandra/pull/2334/files
> > >
> > > I'd really appreciate any kind of review in advance.
> > >
> > >
> > > Despite the number of changes +2,043 −302 and the fact that most of
> > > these additions are related to the tests themselves, I would like to
> > > highlight the crucial design points which are required to make the
> > > SettingsTable virtual table updatable. Some of these have already been
> > > discussed in this thread, and I would like to provide 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Josh McKenzie]

ly think that it should be ok to add a new column.
> > >
> > > I will be very happy to see this landing in 5.0.
> > > I am personally against porting this patch to 4.1. To be clear, I am sure 
> > > you did a great job and my response would be the same to every single 
> > > person - the configuration is quite wide-spread and the devil is in the 
> > > details. I do not see a good reason for exception here except 
> > > convenience. There is no feature flag for these changes too, right?
> > >
> > > Best regards,
> > > Ekaterina
> > >
> > > На четвъртък, 6 юли 2023 г. Miklosovic, Stefan 
> > >  написа:
> > >
> > > Hi Maxim,
> > >
> > > I went through the PR and added my comments. I think David also reviewed 
> > > it. All points you mentioned make sense to me but I humbly think it is 
> > > necessary to have at least one additional pair of eyes on this as the 
> > > patch is relatively impactful.
> > >
> > > I would like to see additional column in system_views.settings of name 
> > > "mutable" and of type "boolean" to see what field I am actually allowed 
> > > to update as an operator.
> > >
> > > It seems to me you agree with the introduction of this column (1) but 
> > > there is no clear agreement where we actually want to put it. You want 
> > > this whole feature to be committed to 4.1 branch as well which is an 
> > > interesting proposal. I was thinking that this work will go to 5.0 only. 
> > > I am not completely sure it is necessary to backport this feature but 
> > > your argumentation here (2) is worth to discuss further.
> > >
> > > If we introduce this change to 4.1, that field would not be there but in 
> > > 5.0 it would. So that way we will not introduce any new column to 
> > > system_views.settings.
> > > We could also go with the introduction of this column to 4.1 if people 
> > > are ok with that.
> > >
> > > For the simplicity, I am slightly leaning towards introducing this 
> > > feature to 5.0 only.
> > >
> > > (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
> > > (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
> > >
> > > 
> > > From: Maxim Muzafarov 
> > > Sent: Friday, June 23, 2023 13:50
> > > To: dev@cassandra.apache.org
> > > Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
> > > running configuration
> > >
> > > NetApp Security WARNING: This is an external email. Do not click links or 
> > > open attachments unless you recognize the sender and know the content is 
> > > safe.
> > >
> > >
> > >
> > >
> > > Hello everyone,
> > >
> > >
> > > As there is a lack of feedback for an option to go on with and having
> > > a discussion for pros and cons for each option I tend to agree with
> > > the vision of this problem proposed by David :-) After a lot of
> > > discussion on Slack, we came to the @ValidatedBy annotation which
> > > points to a validation method of a property and this will address all
> > > our concerns and issues with validation.
> > >
> > > I'd like to raise the visibility of these changes and try to find one
> > > more committer to look at them:
> > > https://issues.apache.org/jira/browse/CASSANDRA-15254
> > > https://github.com/apache/cassandra/pull/2334/files
> > >
> > > I'd really appreciate any kind of review in advance.
> > >
> > >
> > > Despite the number of changes +2,043 −302 and the fact that most of
> > > these additions are related to the tests themselves, I would like to
> > > highlight the crucial design points which are required to make the
> > > SettingsTable virtual table updatable. Some of these have already been
> > > discussed in this thread, and I would like to provide a brief outline
> > > of these points to facilitate the PR review.
> > >
> > > So, what are the problems that have been solved to make the
> > > SettingsTable updatable?
> > >
> > > 1. Input validation.
> > >
> > > Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
> > > the same validation of user input for the same property in their own
> > > ways which fortunately results in a consistent configuration state,
> > > but n

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

t; > Hi Maxim,
> >
> > I went through the PR and added my comments. I think David also reviewed 
> > it. All points you mentioned make sense to me but I humbly think it is 
> > necessary to have at least one additional pair of eyes on this as the patch 
> > is relatively impactful.
> >
> > I would like to see additional column in system_views.settings of name 
> > "mutable" and of type "boolean" to see what field I am actually allowed to 
> > update as an operator.
> >
> > It seems to me you agree with the introduction of this column (1) but there 
> > is no clear agreement where we actually want to put it. You want this whole 
> > feature to be committed to 4.1 branch as well which is an interesting 
> > proposal. I was thinking that this work will go to 5.0 only. I am not 
> > completely sure it is necessary to backport this feature but your 
> > argumentation here (2) is worth to discuss further.
> >
> > If we introduce this change to 4.1, that field would not be there but in 
> > 5.0 it would. So that way we will not introduce any new column to 
> > system_views.settings.
> > We could also go with the introduction of this column to 4.1 if people are 
> > ok with that.
> >
> > For the simplicity, I am slightly leaning towards introducing this feature 
> > to 5.0 only.
> >
> > (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
> > (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
> >
> > 
> > From: Maxim Muzafarov 
> > Sent: Friday, June 23, 2023 13:50
> > To: dev@cassandra.apache.org
> > Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
> > running configuration
> >
> > NetApp Security WARNING: This is an external email. Do not click links or 
> > open attachments unless you recognize the sender and know the content is 
> > safe.
> >
> >
> >
> >
> > Hello everyone,
> >
> >
> > As there is a lack of feedback for an option to go on with and having
> > a discussion for pros and cons for each option I tend to agree with
> > the vision of this problem proposed by David :-) After a lot of
> > discussion on Slack, we came to the @ValidatedBy annotation which
> > points to a validation method of a property and this will address all
> > our concerns and issues with validation.
> >
> > I'd like to raise the visibility of these changes and try to find one
> > more committer to look at them:
> > https://issues.apache.org/jira/browse/CASSANDRA-15254
> > https://github.com/apache/cassandra/pull/2334/files
> >
> > I'd really appreciate any kind of review in advance.
> >
> >
> > Despite the number of changes +2,043 −302 and the fact that most of
> > these additions are related to the tests themselves, I would like to
> > highlight the crucial design points which are required to make the
> > SettingsTable virtual table updatable. Some of these have already been
> > discussed in this thread, and I would like to provide a brief outline
> > of these points to facilitate the PR review.
> >
> > So, what are the problems that have been solved to make the
> > SettingsTable updatable?
> >
> > 1. Input validation.
> >
> > Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
> > the same validation of user input for the same property in their own
> > ways which fortunately results in a consistent configuration state,
> > but not always. The CASSANDRA-17734 is a good example of this.
> >
> > The @ValidatedBy annotations, which point to a validation method have
> > been added to address this particular problem. So, no matter what API
> > is triggered the method will be called to validate input and will also
> > work even if the cassandra.yaml is loaded by the yaml engine in a
> > pre-parse state, such as we are now checking input properties for
> > deprecation and nullability.
> >
> > There are two types of validation worth mentioning:
> > - stateless - properties do not depend on any other configuration;
> > - stateful - properties that require a fully-constructed Config
> > instance to be validated and those values depend on other properties;
> >
> > For the sake of simplicity, the scope of this task will be limited to
> > dealing with stateless properties only, but stateful validations are
> > also supported in the initial PR using property change listeners.
> >
> > 2. Property mutability.
> >
> > There is no way o

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

t; 5.0 only.
>
> (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
> (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
>
> 
> From: Maxim Muzafarov 
> Sent: Friday, June 23, 2023 13:50
> To: dev@cassandra.apache.org
> Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
> running configuration
>
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
>
>
>
>
> Hello everyone,
>
>
> As there is a lack of feedback for an option to go on with and having
> a discussion for pros and cons for each option I tend to agree with
> the vision of this problem proposed by David :-) After a lot of
> discussion on Slack, we came to the @ValidatedBy annotation which
> points to a validation method of a property and this will address all
> our concerns and issues with validation.
>
> I'd like to raise the visibility of these changes and try to find one
> more committer to look at them:
> https://issues.apache.org/jira/browse/CASSANDRA-15254
> https://github.com/apache/cassandra/pull/2334/files
>
> I'd really appreciate any kind of review in advance.
>
>
> Despite the number of changes +2,043 −302 and the fact that most of
> these additions are related to the tests themselves, I would like to
> highlight the crucial design points which are required to make the
> SettingsTable virtual table updatable. Some of these have already been
> discussed in this thread, and I would like to provide a brief outline
> of these points to facilitate the PR review.
>
> So, what are the problems that have been solved to make the
> SettingsTable updatable?
>
> 1. Input validation.
>
> Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
> the same validation of user input for the same property in their own
> ways which fortunately results in a consistent configuration state,
> but not always. The CASSANDRA-17734 is a good example of this.
>
> The @ValidatedBy annotations, which point to a validation method have
> been added to address this particular problem. So, no matter what API
> is triggered the method will be called to validate input and will also
> work even if the cassandra.yaml is loaded by the yaml engine in a
> pre-parse state, such as we are now checking input properties for
> deprecation and nullability.
>
> There are two types of validation worth mentioning:
> - stateless - properties do not depend on any other configuration;
> - stateful - properties that require a fully-constructed Config
> instance to be validated and those values depend on other properties;
>
> For the sake of simplicity, the scope of this task will be limited to
> dealing with stateless properties only, but stateful validations are
> also supported in the initial PR using property change listeners.
>
> 2. Property mutability.
>
> There is no way of distinguishing which parts of a property are
> mutable and which are not. This meta-information must be available at
> runtime and as we discussed earlier the @Mutable annotation is added
> to handle this.
>
> 3. Listening for property changes.
>
> Some of the internal components e.g. CommitLog, may perform some
> operations and/or calculations just before or just after the property
> change. As long as JMX is the only API used to update configuration
> properties, there is no problem. To address this issue the observer
> pattern has been used to maintain the same behaviour.
>
> 4. SettingsTable input/output format.
>
> JMX, SettingsTable and Yaml accept values in different formats which
> may not be compatible in some of the cases especially when
> representing composite objects. The former uses toString() as an
> output, and the latter uses a yaml human-readable format.
>
> So, in order to see the same properties in the same format through
> different APIs, the Yaml representation is reused to display the
> values and to parse a user input in case of update/set operations.
>
> Although the output format between APIs matches in the vast majority
> of cases here is the list of configuration properties that do not
> match:
> - memtable.configurations
> - sstable_formats
> - seed_provider.parameters
> - data_file_directories
>
> The test illustrates the problem:
> https://github.com/apache/cassandra/pull/2334/files#diff-e94bb80f12622412fff9d87b58733e0549ba3024a54714516adc8bc70709933bR319
>
> This could be a regression as the output format is changed, but this
> seems to be the correct change to go with. We can keep it as is, or
> create SettingsTableV2, which seems to be 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Josh McKenzie]

This really is great work Maxim; definitely appreciate all the hard work that's 
gone into it and I think the users will too.

In terms of where it should land, we discussed this type of question at length 
on the ML awhile ago and ended up codifying it in the wiki: 
https://cwiki.apache.org/confluence/display/CASSANDRA/Patching%2C+versioning%2C+and+LTS+releases

> When working on a ticket, use the following guideline to determine which 
> branch to apply it to (Note: See *How To Commit 
> <https://cassandra.apache.org/_/development/how_to_commit.html>* for details 
> on the commit and merge process)
> 
>  • Bugfix: apply to oldest applicable LTS and merge up through latest GA to 
> trunk
>• In the event you need to make changes on the merge commit, merge with 
> *-s ours *and revise the commit via *--amend*
>  • Improvement: apply to *trunk only (next release)*
>• *Note: refactoring and removing dead code qualifies as an Improvement; 
> our priority is stability on GA lines*
>  • New Feature: apply to *trunk only (next release)*
> Our priority is to keep the 2 LTS releases and latest GA stable while 
> releasing new "latest GA" on a cadence that provides new improvements and 
> functionality to users soon enough to be valuable and relevant.
> 

So in this case, target whatever unreleased next feature release (i.e. SEMVER 
MAJOR || MINOR) we have on deck.

On Thu, Jul 6, 2023, at 1:21 PM, Ekaterina Dimitrova wrote:
> Hi,
> 
> First of all, thank you for all the work! 
> I personally think that it should be ok to add a new column.
> 
> I will be very happy to see this landing in 5.0. 
> I am personally against porting this patch to 4.1. To be clear, I am sure you 
> did a great job and my response would be the same to every single person - 
> the configuration is quite wide-spread and the devil is in the details. I do 
> not see a good reason for exception here except convenience. There is no 
> feature flag for these changes too, right?
> 
> Best regards,
> Ekaterina
> 
> На четвъртък, 6 юли 2023 г. Miklosovic, Stefan  
> написа:
>> Hi Maxim,
>> 
>> I went through the PR and added my comments. I think David also reviewed it. 
>> All points you mentioned make sense to me but I humbly think it is necessary 
>> to have at least one additional pair of eyes on this as the patch is 
>> relatively impactful.
>> 
>> I would like to see additional column in system_views.settings of name 
>> "mutable" and of type "boolean" to see what field I am actually allowed to 
>> update as an operator.
>> 
>> It seems to me you agree with the introduction of this column (1) but there 
>> is no clear agreement where we actually want to put it. You want this whole 
>> feature to be committed to 4.1 branch as well which is an interesting 
>> proposal. I was thinking that this work will go to 5.0 only. I am not 
>> completely sure it is necessary to backport this feature but your 
>> argumentation here (2) is worth to discuss further.
>> 
>> If we introduce this change to 4.1, that field would not be there but in 5.0 
>> it would. So that way we will not introduce any new column to 
>> system_views.settings.
>> We could also go with the introduction of this column to 4.1 if people are 
>> ok with that.
>> 
>> For the simplicity, I am slightly leaning towards introducing this feature 
>> to 5.0 only.
>> 
>> (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
>> (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
>> 
>> 
>> From: Maxim Muzafarov 
>> Sent: Friday, June 23, 2023 13:50
>> To: dev@cassandra.apache.org
>> Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
>> running configuration
>> 
>> NetApp Security WARNING: This is an external email. Do not click links or 
>> open attachments unless you recognize the sender and know the content is 
>> safe.
>> 
>> 
>> 
>> 
>> Hello everyone,
>> 
>> 
>> As there is a lack of feedback for an option to go on with and having
>> a discussion for pros and cons for each option I tend to agree with
>> the vision of this problem proposed by David :-) After a lot of
>> discussion on Slack, we came to the @ValidatedBy annotation which
>> points to a validation method of a property and this will address all
>> our concerns and issues with validation.
>> 
>> I'd like to raise the visibility of these changes and try to find one
>> more committer to look at them:
>> https://issues.apache.org/jira/browse/CASSANDRA-

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Ekaterina Dimitrova]

Hi,

First of all, thank you for all the work!
I personally think that it should be ok to add a new column.

I will be very happy to see this landing in 5.0.
I am personally against porting this patch to 4.1. To be clear, I am sure
you did a great job and my response would be the same to every single
person - the configuration is quite wide-spread and the devil is in the
details. I do not see a good reason for exception here except convenience.
There is no feature flag for these changes too, right?

Best regards,
Ekaterina

На четвъртък, 6 юли 2023 г. Miklosovic, Stefan 
написа:

> Hi Maxim,
>
> I went through the PR and added my comments. I think David also reviewed
> it. All points you mentioned make sense to me but I humbly think it is
> necessary to have at least one additional pair of eyes on this as the patch
> is relatively impactful.
>
> I would like to see additional column in system_views.settings of name
> "mutable" and of type "boolean" to see what field I am actually allowed to
> update as an operator.
>
> It seems to me you agree with the introduction of this column (1) but
> there is no clear agreement where we actually want to put it. You want this
> whole feature to be committed to 4.1 branch as well which is an interesting
> proposal. I was thinking that this work will go to 5.0 only. I am not
> completely sure it is necessary to backport this feature but your
> argumentation here (2) is worth to discuss further.
>
> If we introduce this change to 4.1, that field would not be there but in
> 5.0 it would. So that way we will not introduce any new column to
> system_views.settings.
> We could also go with the introduction of this column to 4.1 if people are
> ok with that.
>
> For the simplicity, I am slightly leaning towards introducing this feature
> to 5.0 only.
>
> (1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
> (2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041
>
> 
> From: Maxim Muzafarov 
> Sent: Friday, June 23, 2023 13:50
> To: dev@cassandra.apache.org
> Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change
> running configuration
>
> NetApp Security WARNING: This is an external email. Do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
>
>
>
>
> Hello everyone,
>
>
> As there is a lack of feedback for an option to go on with and having
> a discussion for pros and cons for each option I tend to agree with
> the vision of this problem proposed by David :-) After a lot of
> discussion on Slack, we came to the @ValidatedBy annotation which
> points to a validation method of a property and this will address all
> our concerns and issues with validation.
>
> I'd like to raise the visibility of these changes and try to find one
> more committer to look at them:
> https://issues.apache.org/jira/browse/CASSANDRA-15254
> https://github.com/apache/cassandra/pull/2334/files
>
> I'd really appreciate any kind of review in advance.
>
>
> Despite the number of changes +2,043 −302 and the fact that most of
> these additions are related to the tests themselves, I would like to
> highlight the crucial design points which are required to make the
> SettingsTable virtual table updatable. Some of these have already been
> discussed in this thread, and I would like to provide a brief outline
> of these points to facilitate the PR review.
>
> So, what are the problems that have been solved to make the
> SettingsTable updatable?
>
> 1. Input validation.
>
> Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
> the same validation of user input for the same property in their own
> ways which fortunately results in a consistent configuration state,
> but not always. The CASSANDRA-17734 is a good example of this.
>
> The @ValidatedBy annotations, which point to a validation method have
> been added to address this particular problem. So, no matter what API
> is triggered the method will be called to validate input and will also
> work even if the cassandra.yaml is loaded by the yaml engine in a
> pre-parse state, such as we are now checking input properties for
> deprecation and nullability.
>
> There are two types of validation worth mentioning:
> - stateless - properties do not depend on any other configuration;
> - stateful - properties that require a fully-constructed Config
> instance to be validated and those values depend on other properties;
>
> For the sake of simplicity, the scope of this task will be limited to
> dealing with stateless properties only, but stateful validations are
> also supported in the initial PR usin

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Miklosovic, Stefan]

Hi Maxim,

I went through the PR and added my comments. I think David also reviewed it. 
All points you mentioned make sense to me but I humbly think it is necessary to 
have at least one additional pair of eyes on this as the patch is relatively 
impactful.

I would like to see additional column in system_views.settings of name 
"mutable" and of type "boolean" to see what field I am actually allowed to 
update as an operator.

It seems to me you agree with the introduction of this column (1) but there is 
no clear agreement where we actually want to put it. You want this whole 
feature to be committed to 4.1 branch as well which is an interesting proposal. 
I was thinking that this work will go to 5.0 only. I am not completely sure it 
is necessary to backport this feature but your argumentation here (2) is worth 
to discuss further.

If we introduce this change to 4.1, that field would not be there but in 5.0 it 
would. So that way we will not introduce any new column to 
system_views.settings.
We could also go with the introduction of this column to 4.1 if people are ok 
with that.

For the simplicity, I am slightly leaning towards introducing this feature to 
5.0 only.

(1) https://github.com/apache/cassandra/pull/2334#discussion_r1251104171
(2) https://github.com/apache/cassandra/pull/2334#discussion_r1251248041


From: Maxim Muzafarov 
Sent: Friday, June 23, 2023 13:50
To: dev@cassandra.apache.org
Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change running 
configuration

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.




Hello everyone,


As there is a lack of feedback for an option to go on with and having
a discussion for pros and cons for each option I tend to agree with
the vision of this problem proposed by David :-) After a lot of
discussion on Slack, we came to the @ValidatedBy annotation which
points to a validation method of a property and this will address all
our concerns and issues with validation.

I'd like to raise the visibility of these changes and try to find one
more committer to look at them:
https://issues.apache.org/jira/browse/CASSANDRA-15254
https://github.com/apache/cassandra/pull/2334/files

I'd really appreciate any kind of review in advance.


Despite the number of changes +2,043 −302 and the fact that most of
these additions are related to the tests themselves, I would like to
highlight the crucial design points which are required to make the
SettingsTable virtual table updatable. Some of these have already been
discussed in this thread, and I would like to provide a brief outline
of these points to facilitate the PR review.

So, what are the problems that have been solved to make the
SettingsTable updatable?

1. Input validation.

Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
the same validation of user input for the same property in their own
ways which fortunately results in a consistent configuration state,
but not always. The CASSANDRA-17734 is a good example of this.

The @ValidatedBy annotations, which point to a validation method have
been added to address this particular problem. So, no matter what API
is triggered the method will be called to validate input and will also
work even if the cassandra.yaml is loaded by the yaml engine in a
pre-parse state, such as we are now checking input properties for
deprecation and nullability.

There are two types of validation worth mentioning:
- stateless - properties do not depend on any other configuration;
- stateful - properties that require a fully-constructed Config
instance to be validated and those values depend on other properties;

For the sake of simplicity, the scope of this task will be limited to
dealing with stateless properties only, but stateful validations are
also supported in the initial PR using property change listeners.

2. Property mutability.

There is no way of distinguishing which parts of a property are
mutable and which are not. This meta-information must be available at
runtime and as we discussed earlier the @Mutable annotation is added
to handle this.

3. Listening for property changes.

Some of the internal components e.g. CommitLog, may perform some
operations and/or calculations just before or just after the property
change. As long as JMX is the only API used to update configuration
properties, there is no problem. To address this issue the observer
pattern has been used to maintain the same behaviour.

4. SettingsTable input/output format.

JMX, SettingsTable and Yaml accept values in different formats which
may not be compatible in some of the cases especially when
representing composite objects. The former uses toString() as an
output, and the latter uses a yaml human-readable format.

So, in order to see the same properties in the same format through
different APIs, the

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Hello everyone,


As there is a lack of feedback for an option to go on with and having
a discussion for pros and cons for each option I tend to agree with
the vision of this problem proposed by David :-) After a lot of
discussion on Slack, we came to the @ValidatedBy annotation which
points to a validation method of a property and this will address all
our concerns and issues with validation.

I'd like to raise the visibility of these changes and try to find one
more committer to look at them:
https://issues.apache.org/jira/browse/CASSANDRA-15254
https://github.com/apache/cassandra/pull/2334/files

I'd really appreciate any kind of review in advance.


Despite the number of changes +2,043 −302 and the fact that most of
these additions are related to the tests themselves, I would like to
highlight the crucial design points which are required to make the
SettingsTable virtual table updatable. Some of these have already been
discussed in this thread, and I would like to provide a brief outline
of these points to facilitate the PR review.

So, what are the problems that have been solved to make the
SettingsTable updatable?

1. Input validation.

Currently, the JMX, Yaml and DatabaseDescriptor#apply methods perform
the same validation of user input for the same property in their own
ways which fortunately results in a consistent configuration state,
but not always. The CASSANDRA-17734 is a good example of this.

The @ValidatedBy annotations, which point to a validation method have
been added to address this particular problem. So, no matter what API
is triggered the method will be called to validate input and will also
work even if the cassandra.yaml is loaded by the yaml engine in a
pre-parse state, such as we are now checking input properties for
deprecation and nullability.

There are two types of validation worth mentioning:
- stateless - properties do not depend on any other configuration;
- stateful - properties that require a fully-constructed Config
instance to be validated and those values depend on other properties;

For the sake of simplicity, the scope of this task will be limited to
dealing with stateless properties only, but stateful validations are
also supported in the initial PR using property change listeners.

2. Property mutability.

There is no way of distinguishing which parts of a property are
mutable and which are not. This meta-information must be available at
runtime and as we discussed earlier the @Mutable annotation is added
to handle this.

3. Listening for property changes.

Some of the internal components e.g. CommitLog, may perform some
operations and/or calculations just before or just after the property
change. As long as JMX is the only API used to update configuration
properties, there is no problem. To address this issue the observer
pattern has been used to maintain the same behaviour.

4. SettingsTable input/output format.

JMX, SettingsTable and Yaml accept values in different formats which
may not be compatible in some of the cases especially when
representing composite objects. The former uses toString() as an
output, and the latter uses a yaml human-readable format.

So, in order to see the same properties in the same format through
different APIs, the Yaml representation is reused to display the
values and to parse a user input in case of update/set operations.

Although the output format between APIs matches in the vast majority
of cases here is the list of configuration properties that do not
match:
- memtable.configurations
- sstable_formats
- seed_provider.parameters
- data_file_directories

The test illustrates the problem:
https://github.com/apache/cassandra/pull/2334/files#diff-e94bb80f12622412fff9d87b58733e0549ba3024a54714516adc8bc70709933bR319

This could be a regression as the output format is changed, but this
seems to be the correct change to go with. We can keep it as is, or
create SettingsTableV2, which seems to be unlikely.

The examples of the format change:
-
Property 'seed_provider.parameters' expected:
 {seeds=127.0.0.1:7012}
Property 'seed_provider.parameters' actual:
 seeds: 127.0.0.1:7012
-
Property 'data_file_directories' expected:
 [Ljava.lang.String;@436813f3
Property 'data_file_directories' actual:
 [build/test/cassandra/data]
-

On Mon, 1 May 2023 at 15:11, Maxim Muzafarov  wrote:
>
> Hello everyone,
>
>
> I want to continue this topic and share another properties validation
> option/solution that emerged from my investigation of Cassandra and
> Accord configuration that could be used to make the virtual table
> SettingTable updatable, as each update must move Config from one
> consistent state to another. The solution is based on a few
> assumptions: we don't frequently update the running configuration, and
> we want to base a solution on established Cassandra validation
> approaches to minimise the impact on 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Hello everyone,


I want to continue this topic and share another properties validation
option/solution that emerged from my investigation of Cassandra and
Accord configuration that could be used to make the virtual table
SettingTable updatable, as each update must move Config from one
consistent state to another. The solution is based on a few
assumptions: we don't frequently update the running configuration, and
we want to base a solution on established Cassandra validation
approaches to minimise the impact on the configuration system layer
and thus reuse what we already have.

Cassandra provides a number of methods to check the running
configuration right after it is loaded from the yaml file. Here are
some of them: DatabaseDescriptor#applySSTableFormats,
DatabaseDescriptor#applySimpleConfig,
DatabaseDescriptor#applyPartitioner etc. We can reuse them to perform
consistent configuration updates, as long as applying them to a new
configuration can guarantee us the correctness of the configuration.
They could also help us to set the correct default values, calculated
at runtime, when `null` is passed as an input (or `-1` in the case of
JMX is used). For example, the `concurrent_compactors` has the
following formula to calculate its default: min(8, max(2,
min(getAvailableProcessors(), conf.data_file_directories.length))).
This is unlikely to be achieved, regardless of any external validation
framework we might use.

You can go directly to the code using the link below and see what the
solution looks like, but I think I also need to provide the solution
design with an ideal end state and some alternatives that were
considered.
https://github.com/apache/cassandra/pull/2300/files


= The solution design (reuse methods) =

== Configuration Sources ==

To be able to reuse the methods applySSTableFormats, applySimpleConfig
etc, we need to modify them slightly to pass new configuration changes
for verification. Passing a new instance of the Config class to these
methods to verify a single property on change seems very expensive as
it requires a deep copy of the current configuration instance, so a
good choice here is to create an intermediate interface layer -
ConfigurationSource. Currently, applyXXX methods use direct access to
the fields of the Config class instance, so having an intermediate
interface will allow us to substitute a particular configuration
property at the time of verification and re-run all checks against the
substituted source.

In fact, all of the configuration options that can be used to
configure Cassandra, such as system variables, environment variables
or configuration properties, could be shaded through this interface.
In the end, as the various property sources are implemented using the
same interface, and share the same property names in different
sources, we will be able to do sensible configuration overrides the
same way the Spring does. For instance, read a property from sources
in the following order: system properties, environment variables, and
configuration yaml file.

The ConfigurationSource looks like a flattened source layer:

interface ConfigurationSource {
 T get(Class clazz, String name);
String getString(String name);
Integer getInteger(String name);
Boolean getBoolean(String name);
}

The ConfigurationSource shadowed the following configuration options
in Cassandra:
- the Config class source;
- the CassandraRelevantProperties system properties source;
- the CassandraRelevantEnv environment variables source;
- other sub-project configurations dynamically added to the classpath;


== Configuration Query ==

I have been delving through valuable Cassandra components and process
managers such as StorageService, CommitLog, SnapshotManager etc. and
found a lot of configuration usages doing some boilerplate variable
transformations as well as running some component's actions
before/after changing a configuration property. So this led me to
create a wrapper around the ConfigurationSource to help read values
and listen to changes.

Here are some usage examples:

ConfigurationQuery.from(tableSource)
.getValue(DataStorageSpec.IntMebibytesBound.class,
ConfigFields.REPAIR_SESSION_SPACE)
.map(DataStorageSpec.IntMebibytesBound::toBytes)
.listen(BEFORE_CHANGE, (oldValue, newValue) -> {
assertNotNull(oldValue);
assertNotNull(newValue);
assertEquals(testValue.toBytes(), newValue.intValue());
});

ConfigurationQuery.from(configSource, JMX_EXCEPTION_HANDLER)
.getValue(Integer.class, ConfigFields.CONCURRENT_COMPACTORS)
.listenOptional(ChangeEventType.BEFORE_CHANGE,
(oldValue, newValue) -> newValue.ifPresent(
CompactionManager.instance::setConcurrentCompactors));


= Alternatives =

The least I want to do is reinvent the wheel, so the first thing I did
was look at the configuration frameworks that might help us with the
problems we are discussing.

Whatever framework we consider, the following things need to be taken
into account:
- We have custom 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Hello everyone,


It seems to me that we need another consensus to make the
SettingsTable virtual table updatable. There is an issue with
validating configuration properties that blocks our implementation
with the virtual table.

A short example of validating the values loaded from the YAML file:
- the DurationSpec.LongMillisecondsBound itself requires input quantity >= 0;
- the read_request_timeout Config field with type
DurationSpec.LongMillisecondsBound requires quantity >=
LOWEST_ACCEPTED_TIMEOUT (10ms);

When the read_request_timeout field is modified using JMX, only a
DurationSpec.LongMillisecondsBound type validation is performed and
there is no LOWEST_ACCEPTED_TIMEOUT validation. If we implement the
SettingsTable properties validation in the same way, we just add
another discrepancy.


If we go a little deeper, we are currently validating a configuration
property in the following parts of the code, which makes things even
worse:
- in a property type itself if it's not primitive, e.g.
DataStorageSpec#validateQuantity;
- rarely in nested configuration classes e.g.
AuditLogOptions#validateCategories;
- during the configuration load from yaml-file for null, and non-null,
see YamlConfigurationLoader.PropertiesChecker#check;
- during applying the configuration, e.g. DatabaseDescriptor#applySimpleConfig;
- in DatabaseDescriptor setter methods e.g.
DatabaseDescriptor#setDenylistMaxKeysTotal;
- inside custom classes e.g. SSLFactory#validateSslContext;
- rarely inside JMX methods itself, e.g. StorageService#setRepairRpcTimeout;


To use the same validation path for configuration properties that are
going to be changed through SettingsTable, we need to arrange a common
validation process for each property to rely on, so that the
validation path will be the same regardless of the public interface
used (YAML, JMX, or Virtual Table).

In general, I'd like to avoid building a complex validation framework
for Cassandra's configuration, as the purpose of the project is not to
maintain the configuration itself, so the simpler the validation of
the properties will be, the easier the configuration will be to
maintain.


We might have the following options for building the validation
process, and each of them has its pros and cons:

== 1. ==

Add new annotations to build the property's validation rules (as it
was suggested by David)
@Max, @Min, @NotNull, @Size, @Nullable (already have this one), as
well as custom validators etc.

@Min(5.0) @Max(16.0)
public volatile double phi_convict_threshold = 8.0;

An example of such an approach is the Dropwizard Configuration library
(or Hibernate, Spring)
https://www.dropwizard.io/en/latest/manual/validation.html#annotations


== 2. ==

Add to the @Mutable the set (or single implementation) of validations
it performs, which is closer to what we have now.
As an alternative to having a single class for each constraint, we can
have an enumeration list that stores the same implementations.

public @interface Mutable {
  Class>[] constraints() default {};
}

public class NotNullConstraint implements ConfigurationConstraint {
public void validate(Object newValue) {
if (newValue == null)
throw new IllegalArgumentException("Value cannot be null");
}
}

public class PositiveConstraint implements ConfigurationConstraint {
public void validate(Object newValue) {
if (newValue instanceof Number && ((Number) newValue).intValue() <= 0)
throw new IllegalArgumentException("Value must be positive");
}
}

@Mutable(constraints = { NotNullConstraint.class, PositiveConstraint.class })
public volatile Integer concurrent_compactors;

Something similar is performed for Custom Constraints in Hibernate.
https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#section-constraint-validator


== 3. ==

Enforce setter method names to match the corresponding property name.
This will allow us to call the setter method with reflection, and the
method itself will do all the validation we need.

public volatile int key_cache_keys_to_save;

public static void setKeyCacheKeysToSave(int keyCacheKeysToSave)
{
if (keyCacheKeysToSave < 0)
throw new IllegalArgumentException("key_cache_keys_to_save
must be positive");
conf.key_cache_keys_to_save = keyCacheKeysToSave;
}


I think the options above are the most interesting for us, but if you
have something more appropriate, please share. From my point of view,
option 2 is the most appropriate here, as it fits with everything we
have discussed in this thread. However, they are all fine to go with.

I'm looking forward to hearing your thoughts.

On Tue, 21 Feb 2023 at 22:06, Maxim Muzafarov  wrote:
>
> Hello everyone,
>
>
> I would like to share and discuss the key point of the solution design
> with you before I finalise a pull request with tedious changes
> remaining so that we are all on the same page with the changes to the
> valuable Config class and its accessors.
>
> Here is 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

ned and how to 
> document “groups” of configs (rather than documenting each config, we have a 
> pattern of documenting a feature or pair of configs (such as min/max targets) 
> and showing the configs that can be tweaked).  We did talk about moving to a 
> “nested” config model, but there was concerns about nesting at a feature 
> level as some features are cross cutting (so the “group” of configs may be in 
> different areas), so how we define these “groups” isn’t too clear to me… its 
> also not the common case so maybe less of a concern (if we document 
> row_index_read_size_warn_threshold but not 
> row_index_read_size_fail_threshold, is this still clear?)?
>
> > The SettingsTable may have the following columns and be truly 
> > self-descriptive for a user: name, value, default_value, policy, and 
> > description.
>
>
> If we wish to expose docs in the settings table, this would push us to define 
> these in code and no longer in conf/cassandra.yml… I am ok with this, but 
> this does increase the scope as it needs to address the existing models.  We 
> also need better clarity on compatibility with column additions… there is 
> another dev@ thread pointing out that durable tables cause downgrade issues… 
> but do vtables?  Is it safe to add columns?  I should really bring this 
> question to another thread and have us document….
>
> > On Mar 1, 2023, at 6:33 AM, Maxim Muzafarov  wrote:
> >
> > Thank you all for your replies. Let me add some comments too,
> >
> >
> > From a public API perspective, we have three types of fields in the
> > Config class: internal use only (e.g. logger, PROPERTY_PREFIX prefix),
> > read-only use (e.g. cluster_name), and read-write fields that are
> > currently mutated with JMX. So a single @Mutable annotation is not
> > enough to have clear Config's field separation. Adding two annotations
> > @Mutable and @Immutable might solve the problem, but such an approach
> > leads to code duplication if we want to extend our solution in future
> > with additional parameters such as "description", besides having two
> > different annotations for the same thing might confuse developers who
> > are not familiar with this discussion.
> >
> > So, from my point of view, the best way for us might be as follows
> > mentioned in the PR (the annotation name needs to reflect that the
> > fields are available to the public API and for a user, we can change
> > the name):
> > @Exposure(policy = Exposure.Policy.READ_WRITE)
> > @Exposure(policy = Exposure.Policy.READ_ONLY)
> >
> > Some other names come into my mind: APIAvailable, APIExposed,
> > UserAvailable, UserExposed etc.
> >
> >
> > Stefan mentioned that these annotations could be used to create
> > documentation pages, it's true, I have the same thoughts in mind, and
> > you can see what it will look like at the link below (the description
> > annotation field will be removed from the final version of the PR, but
> > may still be added as part of another issue):
> >
> > https://github.com/apache/cassandra/pull/2133/files#diff-e966f41bc2a418becfe687134ec8cf542eb051eead7fb4917e65a3a2e7c9bce3R392
> >
> > The SettingsTable may have the following columns and be truly
> > self-descriptive for a user: name, value, default_value, policy, and
> > description.
> >
> >
> > Benedict mentioned that we could create a second class to hold such
> > information. The best candidate for this is the ConfigFields class,
> > which is based on the Config class and contains all the field names as
> > constants (I used a small utility class to generate it). But it will
> > still require some manual work, as there is no rule to distinguish
> > which config field is mutable and which isn't. So we would have to
> > update two classes instead of one (the Config class) when adding new
> > configuration fields, which we don't want to do.
> >
> > Here it is in the PR:
> > https://github.com/apache/cassandra/pull/2133/files#diff-fcb4c5bc59d4bb127ffbe9f1ce566b2238c5bb92622da430a4ff879781093d3fR31
> >
> > On Wed, 1 Mar 2023 at 09:21, Miklosovic, Stefan
> >  wrote:
> >>
> >> I am fine with annotations. I am not a big of fan of the generation. From 
> >> my experience whenever we wanted to generate something we had to take care 
> >> of the generator itself and then we had to live with what it generated 
> >> (yeah, that is also a thing) instead of writing it by hand once and have 
> >> some freedom to tweak it however we wanted. Splitting this into the second 
> >> class ... well, I would say that just increases th

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[David Capwell]

ur replies. Let me add some comments too,
> 
> 
> From a public API perspective, we have three types of fields in the
> Config class: internal use only (e.g. logger, PROPERTY_PREFIX prefix),
> read-only use (e.g. cluster_name), and read-write fields that are
> currently mutated with JMX. So a single @Mutable annotation is not
> enough to have clear Config's field separation. Adding two annotations
> @Mutable and @Immutable might solve the problem, but such an approach
> leads to code duplication if we want to extend our solution in future
> with additional parameters such as "description", besides having two
> different annotations for the same thing might confuse developers who
> are not familiar with this discussion.
> 
> So, from my point of view, the best way for us might be as follows
> mentioned in the PR (the annotation name needs to reflect that the
> fields are available to the public API and for a user, we can change
> the name):
> @Exposure(policy = Exposure.Policy.READ_WRITE)
> @Exposure(policy = Exposure.Policy.READ_ONLY)
> 
> Some other names come into my mind: APIAvailable, APIExposed,
> UserAvailable, UserExposed etc.
> 
> 
> Stefan mentioned that these annotations could be used to create
> documentation pages, it's true, I have the same thoughts in mind, and
> you can see what it will look like at the link below (the description
> annotation field will be removed from the final version of the PR, but
> may still be added as part of another issue):
> 
> https://github.com/apache/cassandra/pull/2133/files#diff-e966f41bc2a418becfe687134ec8cf542eb051eead7fb4917e65a3a2e7c9bce3R392
> 
> The SettingsTable may have the following columns and be truly
> self-descriptive for a user: name, value, default_value, policy, and
> description.
> 
> 
> Benedict mentioned that we could create a second class to hold such
> information. The best candidate for this is the ConfigFields class,
> which is based on the Config class and contains all the field names as
> constants (I used a small utility class to generate it). But it will
> still require some manual work, as there is no rule to distinguish
> which config field is mutable and which isn't. So we would have to
> update two classes instead of one (the Config class) when adding new
> configuration fields, which we don't want to do.
> 
> Here it is in the PR:
> https://github.com/apache/cassandra/pull/2133/files#diff-fcb4c5bc59d4bb127ffbe9f1ce566b2238c5bb92622da430a4ff879781093d3fR31
> 
> On Wed, 1 Mar 2023 at 09:21, Miklosovic, Stefan
>  wrote:
>> 
>> I am fine with annotations. I am not a big of fan of the generation. From my 
>> experience whenever we wanted to generate something we had to take care of 
>> the generator itself and then we had to live with what it generated (yeah, 
>> that is also a thing) instead of writing it by hand once and have some 
>> freedom to tweak it however we wanted. Splitting this into the second class 
>> ... well, I would say that just increases the entropy.
>> 
>> We can parse config class on these annotations and produce the documentation 
>> easily. I would probably go so far that I would put that annotation on all 
>> fields. We could have two - Mutable, and Immutable. But that is really 
>> optional.
>> 
>> 
>> From: Benedict 
>> Sent: Wednesday, March 1, 2023 9:09
>> To: dev@cassandra.apache.org
>> Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
>> running configuration
>> 
>> NetApp Security WARNING: This is an external email. Do not click links or 
>> open attachments unless you recognize the sender and know the content is 
>> safe.
>> 
>> 
>> 
>> Another option would be to introduce a second class with the same fields as 
>> the first where we simply specify final for immutable fields, and construct 
>> it after parsing the Config.
>> 
>> We could even generate the non-final version from the one with final fields.
>> 
>> Not sure this would be nicer, but it is an alternative.
>> 
>> On 1 Mar 2023, at 02:10, Ekaterina Dimitrova  wrote:
>> 
>> 
>> I agree with David that the annotations seem a bit too many but if I have to 
>> choose from the three approaches - the annotations one seems most reasonable 
>> to me and I didn’t have the chance to consider any others. Volatile seems 
>> fragile and unclear as a differentiator. I agree
>> 
>> On Tue, 28 Feb 2023 at 17:47, Maxim Muzafarov 
>> mailto:mmu...@apache.org>> wrote:
>> Folks,
>> 
>> If there are no objections to the approach described in this thread,
>>

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Thank you all for your replies. Let me add some comments too,


>From a public API perspective, we have three types of fields in the
Config class: internal use only (e.g. logger, PROPERTY_PREFIX prefix),
read-only use (e.g. cluster_name), and read-write fields that are
currently mutated with JMX. So a single @Mutable annotation is not
enough to have clear Config's field separation. Adding two annotations
@Mutable and @Immutable might solve the problem, but such an approach
leads to code duplication if we want to extend our solution in future
with additional parameters such as "description", besides having two
different annotations for the same thing might confuse developers who
are not familiar with this discussion.

So, from my point of view, the best way for us might be as follows
mentioned in the PR (the annotation name needs to reflect that the
fields are available to the public API and for a user, we can change
the name):
@Exposure(policy = Exposure.Policy.READ_WRITE)
@Exposure(policy = Exposure.Policy.READ_ONLY)

Some other names come into my mind: APIAvailable, APIExposed,
UserAvailable, UserExposed etc.


Stefan mentioned that these annotations could be used to create
documentation pages, it's true, I have the same thoughts in mind, and
you can see what it will look like at the link below (the description
annotation field will be removed from the final version of the PR, but
may still be added as part of another issue):

https://github.com/apache/cassandra/pull/2133/files#diff-e966f41bc2a418becfe687134ec8cf542eb051eead7fb4917e65a3a2e7c9bce3R392

The SettingsTable may have the following columns and be truly
self-descriptive for a user: name, value, default_value, policy, and
description.


Benedict mentioned that we could create a second class to hold such
information. The best candidate for this is the ConfigFields class,
which is based on the Config class and contains all the field names as
constants (I used a small utility class to generate it). But it will
still require some manual work, as there is no rule to distinguish
which config field is mutable and which isn't. So we would have to
update two classes instead of one (the Config class) when adding new
configuration fields, which we don't want to do.

Here it is in the PR:
https://github.com/apache/cassandra/pull/2133/files#diff-fcb4c5bc59d4bb127ffbe9f1ce566b2238c5bb92622da430a4ff879781093d3fR31

On Wed, 1 Mar 2023 at 09:21, Miklosovic, Stefan
 wrote:
>
> I am fine with annotations. I am not a big of fan of the generation. From my 
> experience whenever we wanted to generate something we had to take care of 
> the generator itself and then we had to live with what it generated (yeah, 
> that is also a thing) instead of writing it by hand once and have some 
> freedom to tweak it however we wanted. Splitting this into the second class 
> ... well, I would say that just increases the entropy.
>
> We can parse config class on these annotations and produce the documentation 
> easily. I would probably go so far that I would put that annotation on all 
> fields. We could have two - Mutable, and Immutable. But that is really 
> optional.
>
> 
> From: Benedict 
> Sent: Wednesday, March 1, 2023 9:09
> To: dev@cassandra.apache.org
> Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change 
> running configuration
>
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
>
>
>
> Another option would be to introduce a second class with the same fields as 
> the first where we simply specify final for immutable fields, and construct 
> it after parsing the Config.
>
> We could even generate the non-final version from the one with final fields.
>
> Not sure this would be nicer, but it is an alternative.
>
> On 1 Mar 2023, at 02:10, Ekaterina Dimitrova  wrote:
>
> 
> I agree with David that the annotations seem a bit too many but if I have to 
> choose from the three approaches - the annotations one seems most reasonable 
> to me and I didn’t have the chance to consider any others. Volatile seems 
> fragile and unclear as a differentiator. I agree
>
> On Tue, 28 Feb 2023 at 17:47, Maxim Muzafarov 
> mailto:mmu...@apache.org>> wrote:
> Folks,
>
> If there are no objections to the approach described in this thread,
> I'd like to proceed with this change. The change seems to be valuable
> for the upcoming release, so any comments are really appreciated.
>
> On Wed, 22 Feb 2023 at 21:51, David Capwell 
> mailto:dcapw...@apple.com>> wrote:
> >
> > I guess back to the point of the thread, we need a way to know what configs 
> > are mutable for the settings virtual table, so need some way to denote that 
> &

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Miklosovic, Stefan]

I am fine with annotations. I am not a big of fan of the generation. From my 
experience whenever we wanted to generate something we had to take care of the 
generator itself and then we had to live with what it generated (yeah, that is 
also a thing) instead of writing it by hand once and have some freedom to tweak 
it however we wanted. Splitting this into the second class ... well, I would 
say that just increases the entropy.

We can parse config class on these annotations and produce the documentation 
easily. I would probably go so far that I would put that annotation on all 
fields. We could have two - Mutable, and Immutable. But that is really optional.


From: Benedict 
Sent: Wednesday, March 1, 2023 9:09
To: dev@cassandra.apache.org
Subject: Re: [DISCUSS] Allow UPDATE on settings virtual table to change running 
configuration

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



Another option would be to introduce a second class with the same fields as the 
first where we simply specify final for immutable fields, and construct it 
after parsing the Config.

We could even generate the non-final version from the one with final fields.

Not sure this would be nicer, but it is an alternative.

On 1 Mar 2023, at 02:10, Ekaterina Dimitrova  wrote:


I agree with David that the annotations seem a bit too many but if I have to 
choose from the three approaches - the annotations one seems most reasonable to 
me and I didn’t have the chance to consider any others. Volatile seems fragile 
and unclear as a differentiator. I agree

On Tue, 28 Feb 2023 at 17:47, Maxim Muzafarov 
mailto:mmu...@apache.org>> wrote:
Folks,

If there are no objections to the approach described in this thread,
I'd like to proceed with this change. The change seems to be valuable
for the upcoming release, so any comments are really appreciated.

On Wed, 22 Feb 2023 at 21:51, David Capwell 
mailto:dcapw...@apple.com>> wrote:
>
> I guess back to the point of the thread, we need a way to know what configs 
> are mutable for the settings virtual table, so need some way to denote that 
> the config replica_filtering_protection.cached_rows_fail_threshold is 
> mutable.  Given the way that the yaml config works, we can’t rely on the 
> presences of “final” or not, so need some way to mark a config is mutable for 
> that table, does anyone want to offer feedback on what works best for them?
>
> Out of all proposals given so far “volatile” is the least verbose but also 
> not explicit (as this thread is showing there is debate on if this should be 
> present), new annotations are a little more verbose but would be explicit (no 
> surprises), and getter/setters in different classes (such as DD) is the most 
> verbose and suffers from not being explicit and ambiguity for mapping back to 
> Config.
>
> Given the above, annotations sounds like the best option, but do we really 
> want our config to look as follows?
>
> @Replaces(oldName = "native_transport_idle_timeout_in_ms", converter = 
> Converters.MILLIS_DURATION_LONG, deprecated = true)
> @Mutable
> public DurationSpec.LongMillisecondsBound native_transport_idle_timeout = new 
> DurationSpec.LongMillisecondsBound("0ms”);
> @Mutable
> public DurationSpec.LongMillisecondsBound transaction_timeout = new 
> DurationSpec.LongMillisecondsBound("30s”);
> @Mutable
> public double phi_convict_threshold = 8.0;
> public String partitioner; // assume immutable by default?
>
>
> > On Feb 22, 2023, at 6:20 AM, Benedict 
> > mailto:bened...@apache.org>> wrote:
> >
> > Could you describe the issues? Config that is globally exposed should 
> > ideally be immutable with final members, in which case volatile is only 
> > necessary if you’re using the config parameter in a tight loop that you 
> > need to witness a new value - which shouldn’t apply to any of our config.
> >
> > There are some weird niches, like updating long values on some (unsupported 
> > by us) JVMs that may tear. Technically you also require it for visibility 
> > with the JMM. But in practice it is mostly unnecessary. Often what seems to 
> > be a volatile issue is really something else.
> >
> >> On 22 Feb 2023, at 13:18, Benjamin Lerer 
> >> mailto:b.le...@gmail.com>> wrote:
> >>
> >> I have seen issues with some updatable parameters which were missing the 
> >> volatile keyword.
> >>
> >> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko 
> >> mailto:alek...@apple.com>> a écrit :
> >> FWIW most of those volatile fields, if not in fact all of them, should NOT 
> >> be volatil

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Benedict]

Another option would be to introduce a second class with the same fields as the first where we simply specify final for immutable fields, and construct it after parsing the Config.We could even generate the non-final version from the one with final fields.Not sure this would be nicer, but it is an alternative.On 1 Mar 2023, at 02:10, Ekaterina Dimitrova  wrote:I agree with David that the annotations seem a bit too many but if I have to choose from the three approaches - the annotations one seems most reasonable to me and I didn’t have the chance to consider any others. Volatile seems fragile and unclear as a differentiator. I agreeOn Tue, 28 Feb 2023 at 17:47, Maxim Muzafarov  wrote:Folks,

If there are no objections to the approach described in this thread,
I'd like to proceed with this change. The change seems to be valuable
for the upcoming release, so any comments are really appreciated.

On Wed, 22 Feb 2023 at 21:51, David Capwell  wrote:
>
> I guess back to the point of the thread, we need a way to know what configs are mutable for the settings virtual table, so need some way to denote that the config replica_filtering_protection.cached_rows_fail_threshold is mutable.  Given the way that the yaml config works, we can’t rely on the presences of “final” or not, so need some way to mark a config is mutable for that table, does anyone want to offer feedback on what works best for them?
>
> Out of all proposals given so far “volatile” is the least verbose but also not explicit (as this thread is showing there is debate on if this should be present), new annotations are a little more verbose but would be explicit (no surprises), and getter/setters in different classes (such as DD) is the most verbose and suffers from not being explicit and ambiguity for mapping back to Config.
>
> Given the above, annotations sounds like the best option, but do we really want our config to look as follows?
>
> @Replaces(oldName = "native_transport_idle_timeout_in_ms", converter = Converters.MILLIS_DURATION_LONG, deprecated = true)
> @Mutable
> public DurationSpec.LongMillisecondsBound native_transport_idle_timeout = new DurationSpec.LongMillisecondsBound("0ms”);
> @Mutable
> public DurationSpec.LongMillisecondsBound transaction_timeout = new DurationSpec.LongMillisecondsBound("30s”);
> @Mutable
> public double phi_convict_threshold = 8.0;
> public String partitioner; // assume immutable by default?
>
>
> > On Feb 22, 2023, at 6:20 AM, Benedict  wrote:
> >
> > Could you describe the issues? Config that is globally exposed should ideally be immutable with final members, in which case volatile is only necessary if you’re using the config parameter in a tight loop that you need to witness a new value - which shouldn’t apply to any of our config.
> >
> > There are some weird niches, like updating long values on some (unsupported by us) JVMs that may tear. Technically you also require it for visibility with the JMM. But in practice it is mostly unnecessary. Often what seems to be a volatile issue is really something else.
> >
> >> On 22 Feb 2023, at 13:18, Benjamin Lerer  wrote:
> >>
> >> I have seen issues with some updatable parameters which were missing the volatile keyword.
> >>
> >> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  a écrit :
> >> FWIW most of those volatile fields, if not in fact all of them, should NOT be volatile at all. Someone started the trend and most folks have been copycatting or doing the same for consistency with the rest of the codebase.
> >>
> >> Please definitely don’t rely on that.
> >>
> >>> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
> >>>
> >>> 1. Rely on the volatile keyword in front of fields in the Config class;
> >>>
> >>> I would say this is the most confusing option for me because it
> >>> doesn't give us all the guarantees we need, and also:
> >>> - We have no explicit control over what exactly we expose to a user.
> >>> When we modify the JMX API, we're implementing a new method for the
> >>> MBean, which in turn makes this action an explicit exposure;
> >>> - The volatile keyword is not the only way to achieve thread safety,
> >>> and looks strange for the public API design point;
> >>> - A good example is the setEnableDropCompactStorage method, which
> >>> changes the volatile field, but is only visible for testing purposes;
> >>
> >>
>

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Ekaterina Dimitrova]

I agree with David that the annotations seem a bit too many but if I have
to choose from the three approaches - the annotations one seems most
reasonable to me and I didn’t have the chance to consider any others.
Volatile seems fragile and unclear as a differentiator. I agree

On Tue, 28 Feb 2023 at 17:47, Maxim Muzafarov  wrote:

> Folks,
>
> If there are no objections to the approach described in this thread,
> I'd like to proceed with this change. The change seems to be valuable
> for the upcoming release, so any comments are really appreciated.
>
> On Wed, 22 Feb 2023 at 21:51, David Capwell  wrote:
> >
> > I guess back to the point of the thread, we need a way to know what
> configs are mutable for the settings virtual table, so need some way to
> denote that the config
> replica_filtering_protection.cached_rows_fail_threshold is mutable.  Given
> the way that the yaml config works, we can’t rely on the presences of
> “final” or not, so need some way to mark a config is mutable for that
> table, does anyone want to offer feedback on what works best for them?
> >
> > Out of all proposals given so far “volatile” is the least verbose but
> also not explicit (as this thread is showing there is debate on if this
> should be present), new annotations are a little more verbose but would be
> explicit (no surprises), and getter/setters in different classes (such as
> DD) is the most verbose and suffers from not being explicit and ambiguity
> for mapping back to Config.
> >
> > Given the above, annotations sounds like the best option, but do we
> really want our config to look as follows?
> >
> > @Replaces(oldName = "native_transport_idle_timeout_in_ms", converter =
> Converters.MILLIS_DURATION_LONG, deprecated = true)
> > @Mutable
> > public DurationSpec.LongMillisecondsBound native_transport_idle_timeout
> = new DurationSpec.LongMillisecondsBound("0ms”);
> > @Mutable
> > public DurationSpec.LongMillisecondsBound transaction_timeout = new
> DurationSpec.LongMillisecondsBound("30s”);
> > @Mutable
> > public double phi_convict_threshold = 8.0;
> > public String partitioner; // assume immutable by default?
> >
> >
> > > On Feb 22, 2023, at 6:20 AM, Benedict  wrote:
> > >
> > > Could you describe the issues? Config that is globally exposed should
> ideally be immutable with final members, in which case volatile is only
> necessary if you’re using the config parameter in a tight loop that you
> need to witness a new value - which shouldn’t apply to any of our config.
> > >
> > > There are some weird niches, like updating long values on some
> (unsupported by us) JVMs that may tear. Technically you also require it for
> visibility with the JMM. But in practice it is mostly unnecessary. Often
> what seems to be a volatile issue is really something else.
> > >
> > >> On 22 Feb 2023, at 13:18, Benjamin Lerer  wrote:
> > >>
> > >> I have seen issues with some updatable parameters which were missing
> the volatile keyword.
> > >>
> > >> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko 
> a écrit :
> > >> FWIW most of those volatile fields, if not in fact all of them,
> should NOT be volatile at all. Someone started the trend and most folks
> have been copycatting or doing the same for consistency with the rest of
> the codebase.
> > >>
> > >> Please definitely don’t rely on that.
> > >>
> > >>> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
> > >>>
> > >>> 1. Rely on the volatile keyword in front of fields in the Config
> class;
> > >>>
> > >>> I would say this is the most confusing option for me because it
> > >>> doesn't give us all the guarantees we need, and also:
> > >>> - We have no explicit control over what exactly we expose to a user.
> > >>> When we modify the JMX API, we're implementing a new method for the
> > >>> MBean, which in turn makes this action an explicit exposure;
> > >>> - The volatile keyword is not the only way to achieve thread safety,
> > >>> and looks strange for the public API design point;
> > >>> - A good example is the setEnableDropCompactStorage method, which
> > >>> changes the volatile field, but is only visible for testing purposes;
> > >>
> > >>
> >
>

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Folks,

If there are no objections to the approach described in this thread,
I'd like to proceed with this change. The change seems to be valuable
for the upcoming release, so any comments are really appreciated.

On Wed, 22 Feb 2023 at 21:51, David Capwell  wrote:
>
> I guess back to the point of the thread, we need a way to know what configs 
> are mutable for the settings virtual table, so need some way to denote that 
> the config replica_filtering_protection.cached_rows_fail_threshold is 
> mutable.  Given the way that the yaml config works, we can’t rely on the 
> presences of “final” or not, so need some way to mark a config is mutable for 
> that table, does anyone want to offer feedback on what works best for them?
>
> Out of all proposals given so far “volatile” is the least verbose but also 
> not explicit (as this thread is showing there is debate on if this should be 
> present), new annotations are a little more verbose but would be explicit (no 
> surprises), and getter/setters in different classes (such as DD) is the most 
> verbose and suffers from not being explicit and ambiguity for mapping back to 
> Config.
>
> Given the above, annotations sounds like the best option, but do we really 
> want our config to look as follows?
>
> @Replaces(oldName = "native_transport_idle_timeout_in_ms", converter = 
> Converters.MILLIS_DURATION_LONG, deprecated = true)
> @Mutable
> public DurationSpec.LongMillisecondsBound native_transport_idle_timeout = new 
> DurationSpec.LongMillisecondsBound("0ms”);
> @Mutable
> public DurationSpec.LongMillisecondsBound transaction_timeout = new 
> DurationSpec.LongMillisecondsBound("30s”);
> @Mutable
> public double phi_convict_threshold = 8.0;
> public String partitioner; // assume immutable by default?
>
>
> > On Feb 22, 2023, at 6:20 AM, Benedict  wrote:
> >
> > Could you describe the issues? Config that is globally exposed should 
> > ideally be immutable with final members, in which case volatile is only 
> > necessary if you’re using the config parameter in a tight loop that you 
> > need to witness a new value - which shouldn’t apply to any of our config.
> >
> > There are some weird niches, like updating long values on some (unsupported 
> > by us) JVMs that may tear. Technically you also require it for visibility 
> > with the JMM. But in practice it is mostly unnecessary. Often what seems to 
> > be a volatile issue is really something else.
> >
> >> On 22 Feb 2023, at 13:18, Benjamin Lerer  wrote:
> >>
> >> I have seen issues with some updatable parameters which were missing the 
> >> volatile keyword.
> >>
> >> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  a 
> >> écrit :
> >> FWIW most of those volatile fields, if not in fact all of them, should NOT 
> >> be volatile at all. Someone started the trend and most folks have been 
> >> copycatting or doing the same for consistency with the rest of the 
> >> codebase.
> >>
> >> Please definitely don’t rely on that.
> >>
> >>> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
> >>>
> >>> 1. Rely on the volatile keyword in front of fields in the Config class;
> >>>
> >>> I would say this is the most confusing option for me because it
> >>> doesn't give us all the guarantees we need, and also:
> >>> - We have no explicit control over what exactly we expose to a user.
> >>> When we modify the JMX API, we're implementing a new method for the
> >>> MBean, which in turn makes this action an explicit exposure;
> >>> - The volatile keyword is not the only way to achieve thread safety,
> >>> and looks strange for the public API design point;
> >>> - A good example is the setEnableDropCompactStorage method, which
> >>> changes the volatile field, but is only visible for testing purposes;
> >>
> >>
>

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[David Capwell]

I guess back to the point of the thread, we need a way to know what configs are 
mutable for the settings virtual table, so need some way to denote that the 
config replica_filtering_protection.cached_rows_fail_threshold is mutable.  
Given the way that the yaml config works, we can’t rely on the presences of 
“final” or not, so need some way to mark a config is mutable for that table, 
does anyone want to offer feedback on what works best for them?

Out of all proposals given so far “volatile” is the least verbose but also not 
explicit (as this thread is showing there is debate on if this should be 
present), new annotations are a little more verbose but would be explicit (no 
surprises), and getter/setters in different classes (such as DD) is the most 
verbose and suffers from not being explicit and ambiguity for mapping back to 
Config.   

Given the above, annotations sounds like the best option, but do we really want 
our config to look as follows?

@Replaces(oldName = "native_transport_idle_timeout_in_ms", converter = 
Converters.MILLIS_DURATION_LONG, deprecated = true)
@Mutable
public DurationSpec.LongMillisecondsBound native_transport_idle_timeout = new 
DurationSpec.LongMillisecondsBound("0ms”);
@Mutable
public DurationSpec.LongMillisecondsBound transaction_timeout = new 
DurationSpec.LongMillisecondsBound("30s”);
@Mutable
public double phi_convict_threshold = 8.0;
public String partitioner; // assume immutable by default?


> On Feb 22, 2023, at 6:20 AM, Benedict  wrote:
> 
> Could you describe the issues? Config that is globally exposed should ideally 
> be immutable with final members, in which case volatile is only necessary if 
> you’re using the config parameter in a tight loop that you need to witness a 
> new value - which shouldn’t apply to any of our config.
> 
> There are some weird niches, like updating long values on some (unsupported 
> by us) JVMs that may tear. Technically you also require it for visibility 
> with the JMM. But in practice it is mostly unnecessary. Often what seems to 
> be a volatile issue is really something else.
> 
>> On 22 Feb 2023, at 13:18, Benjamin Lerer  wrote:
>> 
>> I have seen issues with some updatable parameters which were missing the 
>> volatile keyword.
>> 
>> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  a 
>> écrit :
>> FWIW most of those volatile fields, if not in fact all of them, should NOT 
>> be volatile at all. Someone started the trend and most folks have been 
>> copycatting or doing the same for consistency with the rest of the codebase.
>> 
>> Please definitely don’t rely on that.
>> 
>>> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
>>> 
>>> 1. Rely on the volatile keyword in front of fields in the Config class;
>>> 
>>> I would say this is the most confusing option for me because it
>>> doesn't give us all the guarantees we need, and also:
>>> - We have no explicit control over what exactly we expose to a user.
>>> When we modify the JMX API, we're implementing a new method for the
>>> MBean, which in turn makes this action an explicit exposure;
>>> - The volatile keyword is not the only way to achieve thread safety,
>>> and looks strange for the public API design point;
>>> - A good example is the setEnableDropCompactStorage method, which
>>> changes the volatile field, but is only visible for testing purposes;
>> 
>> 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Benedict]

Could you describe the issues? Config that is globally exposed should ideally be immutable with final members, in which case volatile is only necessary if you’re using the config parameter in a tight loop that you need to witness a new value - which shouldn’t apply to any of our config.There are some weird niches, like updating long values on some (unsupported by us) JVMs that may tear. Technically you also require it for visibility with the JMM. But in practice it is mostly unnecessary. Often what seems to be a volatile issue is really something else.On 22 Feb 2023, at 13:18, Benjamin Lerer  wrote:I have seen issues with some updatable parameters which were missing the volatile keyword.Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  a écrit :FWIW most of those volatile fields, if not in fact all of them, should NOT be volatile at all. Someone started the trend and most folks have been copycatting or doing the same for consistency with the rest of the codebase.Please definitely don’t rely on that.On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:1. Rely on the volatile keyword in front of fields in the Config class;I would say this is the most confusing option for me because itdoesn't give us all the guarantees we need, and also:- We have no explicit control over what exactly we expose to a user.When we modify the JMX API, we're implementing a new method for theMBean, which in turn makes this action an explicit exposure;- The volatile keyword is not the only way to achieve thread safety,and looks strange for the public API design point;- A good example is the setEnableDropCompactStorage method, whichchanges the volatile field, but is only visible for testing purposes;

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Aleksey Yeshchenko]

Could maybe be an issue for some really tight unit tests. In actual use the 
updates to those fields will be globally visible near instantly without 
volatile keyword.

> On 22 Feb 2023, at 13:17, Benjamin Lerer  wrote:
> 
> I have seen issues with some updatable parameters which were missing the 
> volatile keyword.
> 
> Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  > a écrit :
>> FWIW most of those volatile fields, if not in fact all of them, should NOT 
>> be volatile at all. Someone started the trend and most folks have been 
>> copycatting or doing the same for consistency with the rest of the codebase.
>> 
>> Please definitely don’t rely on that.
>> 
>>> On 21 Feb 2023, at 21:06, Maxim Muzafarov >> > wrote:
>>> 
>>> 1. Rely on the volatile keyword in front of fields in the Config class;
>>> 
>>> I would say this is the most confusing option for me because it
>>> doesn't give us all the guarantees we need, and also:
>>> - We have no explicit control over what exactly we expose to a user.
>>> When we modify the JMX API, we're implementing a new method for the
>>> MBean, which in turn makes this action an explicit exposure;
>>> - The volatile keyword is not the only way to achieve thread safety,
>>> and looks strange for the public API design point;
>>> - A good example is the setEnableDropCompactStorage method, which
>>> changes the volatile field, but is only visible for testing purposes;
>> 

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Benjamin Lerer]

I have seen issues with some updatable parameters which were missing the
volatile keyword.

Le mer. 22 févr. 2023 à 11:36, Aleksey Yeshchenko  a
écrit :

> FWIW most of those volatile fields, if not in fact all of them, should NOT
> be volatile at all. Someone started the trend and most folks have been
> copycatting or doing the same for consistency with the rest of the codebase.
>
> Please definitely don’t rely on that.
>
> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
>
> 1. Rely on the volatile keyword in front of fields in the Config class;
>
> I would say this is the most confusing option for me because it
> doesn't give us all the guarantees we need, and also:
> - We have no explicit control over what exactly we expose to a user.
> When we modify the JMX API, we're implementing a new method for the
> MBean, which in turn makes this action an explicit exposure;
> - The volatile keyword is not the only way to achieve thread safety,
> and looks strange for the public API design point;
> - A good example is the setEnableDropCompactStorage method, which
> changes the volatile field, but is only visible for testing purposes;
>
>
>

Re: [DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Aleksey Yeshchenko]

FWIW most of those volatile fields, if not in fact all of them, should NOT be 
volatile at all. Someone started the trend and most folks have been copycatting 
or doing the same for consistency with the rest of the codebase.

Please definitely don’t rely on that.

> On 21 Feb 2023, at 21:06, Maxim Muzafarov  wrote:
> 
> 1. Rely on the volatile keyword in front of fields in the Config class;
> 
> I would say this is the most confusing option for me because it
> doesn't give us all the guarantees we need, and also:
> - We have no explicit control over what exactly we expose to a user.
> When we modify the JMX API, we're implementing a new method for the
> MBean, which in turn makes this action an explicit exposure;
> - The volatile keyword is not the only way to achieve thread safety,
> and looks strange for the public API design point;
> - A good example is the setEnableDropCompactStorage method, which
> changes the volatile field, but is only visible for testing purposes;

[DISCUSS] Allow UPDATE on settings virtual table to change running configuration

[Maxim Muzafarov]

Hello everyone,


I would like to share and discuss the key point of the solution design
with you before I finalise a pull request with tedious changes
remaining so that we are all on the same page with the changes to the
valuable Config class and its accessors.

Here is the issue I'm working on:
"Allow UPDATE on settings virtual table to change running configurations".
https://issues.apache.org/jira/browse/CASSANDRA-15254

Below is the restricted solution design at a very high level, all the
details have been discussed in the related JIRA issue.


= What we have now =

- We use JMX MBeans to mutate this runtime configuration during the
node run or to view the configuration values. Some of the JMX MBean
methods use camel case to match configuration field names;
- We use the SettingsTable only to view configuration values at
runtime, but we are not able to mutate the configuration through it;
- We load the configuration from cassandra.yaml into the Config class
instance during the node bootstrap (is accessed with
DatabaseDescriptor, GuardrailsOptions);
- The Config class itself has nested configurations such as
ReplicaFilteringProtectionOptions (it is important to keep this always
in mind);


= What we want to achieve =

We want to use the SettingsTable virtual table to change the runtime
configuration, as we do it now with JMX MBeans, and:
- If the affected component is updated (or the component's logic is
executed) before or after the property change, we want to keep this
behaviour for the virtual table for the same configuration property;
- We want to ensure consistency of such changes between the virtual
table API and the JMX API used;


= The main question =

To enable configuration management with the virtual table, we need to
know the answer to the following key question:
- How can we be sure to determine at runtime which of the properties
we can change and which we can't?


= Options for an answer to the question above =

1. Rely on the volatile keyword in front of fields in the Config class;

I would say this is the most confusing option for me because it
doesn't give us all the guarantees we need, and also:
- We have no explicit control over what exactly we expose to a user.
When we modify the JMX API, we're implementing a new method for the
MBean, which in turn makes this action an explicit exposure;
- The volatile keyword is not the only way to achieve thread safety,
and looks strange for the public API design point;
- A good example is the setEnableDropCompactStorage method, which
changes the volatile field, but is only visible for testing purposes;

2. Annotation-based exposition.

I have created Exposure(Exposure.Policy.READ_ONLY),
Exposure(Exposure.Policy.READ_WRITE) annotations to mark all the
configuration fields we are going to expose to the public API (JMX, as
well as the SettingsTable) in the Config class. All the configuration
fields (in the Config class and any nested classes) that we want to
expose (and already are used by JMX) need to tag with an annotation of
the appropriate type.

The most confusing thing here, apart from the number of tedious
changes: we are using reflection to mutate configuration field values
at runtime, which makes some of the fields look "unused" in the IDE.
This can be not very pleasant for developers looking at the Config
class for the first time.

You can find the PR related to this type of change here (only a few
configuration fields have been annotated for the visibility of all
changes):
https://github.com/apache/cassandra/pull/2133/files


3. Enforce setter/getter method name rules by converting these methods
in camel case to the field name with underscores.

To rely on setter methods, we need to enforce the naming rules of the
setters. I have collected information about which field names match
their camel case getter/setter methods:

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

The most confusing part of this type of change is the number of
changes in additional classes according to the calculation above and
some difficulties with enforcing this rule for nested configuration
classes.

Find out what this change is about here:
https://github.com/apache/cassandra/pull/2172/files


= Summary =

In summary, from my point of view, the annotation approach will be the
most robust solution for us, so I'd like to continue with it. It also
provides an easy way to extend the SettingTable with additional
columns such as runtime type (READ_ONLY, READ_WRITE) and a description
column. This ends up looking much more user-friendly.

Another advantage of the annotation approach is that we can rely on
this annotation to generate dedicated dynamic JMX beans that only
respond to node configuration management to avoid any inconsistencies
like those mentioned here [2] (I have described a similar approach
here [1], but for metrics). But