Is the primary pain point you're trying to solve getting a 2nd committer reviewer Maxim? And / or making the review process simpler / cleaner for someone?
On Wed, Oct 18, 2023, at 5:06 PM, Maxim Muzafarov wrote: > Hello everyone, > > It has been a long time since the last update on this thread, so I > wanted to share some status updates: The issue is still awaiting > review, but all my hopes are pinned on Benjamin :-) > > My question here is, is there anything I can do to facilitate the > review for anyone who wants to delve into the patch? > > I have a few thoughts to follow: > - CEPify the changes - this will allow us to see the result of the > discussion on a single page without having to re-read the whole > thread; > - Write a blog post with possible design solutions - this will both > reveal the results of the discussion and potentially will draw some > attention to the community; > - Presenting and discussing slides at one of the Cassandra Town Halls; > > I tend to the 1-st and/or 2-nd points. What are the best practices we > have here for such cases though? Any thoughts? > > On Tue, 11 Jul 2023 at 15:51, Maxim Muzafarov <mmu...@apache.org> wrote: > > > > Thank you for your comments and for sharing the ticket targeting > > strategy, I'm really happy to see this page where I have found all the > > answers to the questions I had. So, I tend towards your view and will > > just land this ticket on the 5.0 release only for now as it makes > > sense for me as well. > > > > I didn't add the feature flag for this feature because for 99% of the > > source code changes it only works with Cassandra internals leaving the > > public API unchanged. A few remarks on this are: > > - the display format of the vtable property has changed to match the > > yaml configuration style, this doesn't mean that we are displaying > > property values in a completely different way in fact the formats > > match with only 4 exceptions mentioned in the message above (this > > should be fine for the major release I hope); > > - a new column, which we've agreed to add (I'll fix the PR shortly); > > > > > > I would also like to mention the follow-up todos required by this > > issue to set the right expectations. Currently, we've brought a few > > properties under the framework to make them updateable with the > > SettingsTable, so that you can keep focusing on the framework itself > > rather than on tagging the configuration properties themselves with > > the @Mutable annotation. Although the solution is self-sufficient for > > the already tagged properties, we still need to bring the rest of them > > under the framework afterwards. I'll create an issue and do it right, > > we'll be done with the inital patch. > > > > > > On Fri, 7 Jul 2023 at 20:37, Josh McKenzie <jmcken...@apache.org> wrote: > > > > > > 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 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 > > > <stefan.mikloso...@netapp.com> написа: > > > > > > 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 <mmu...@apache.org> > > > 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 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 <mmu...@apache.org> 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 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> T get(Class<T> 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 configuration datatypes such as DataStorageSpec, > > > > DataStorageSpec; > > > > - We have custom DurationSpec, so we either move them to Duration, > > > > preserving backwards compatibility for all supported APIs (yaml, JMX), > > > > or extend a considered framework with new types, we have to provide > > > > data type converters in the latter case; > > > > - An additional dependency, so the key component (configuration) of > > > > the project becomes dependent on an external library version; > > > > - We have to deal with configuration defaults calculated during > > > > initialisation to maintain backward compatibility; > > > > > > > > The frameworks I have looked at: > > > > - commons-configuration > > > > https://github.com/apache/commons-configuration > > > > - lightbend config > > > > https://github.com/lightbend/config > > > > - Netflix archaius > > > > https://github.com/Netflix/archaius > > > > > > > > > > > > The Apache Commons configuration from this list sounds less risky to > > > > us as we already have dependencies like commons-codec, commons-cli > > > > etc. The approach of how configuration fields are used in the > > > > Cassandra project is closer to the way the commons-configuration > > > > library maintains them, so we can replace the ConfigurationSource > > > > layer from the design with AbstractConfiguration > > > > (commons-configuration), keeping the same properties validation design > > > > concept. > > > > > > > > The Apache Commons configuration provides Duration configuration types > > > > that look similar to the DurationSpec in Cassandra. Support/having > > > > both types in the case of we're going this library for the same > > > > abstraction confuses those who will be dealing with the configuration > > > > API in the internal code, so some kind of migration is still required > > > > here as well as creating custom adapters to support backwards > > > > compatibility. This is a HUGE change that helps to create an API for > > > > internal configuration usage for both Cassandra and sub-projects (e.g. > > > > Accord), but still does not solve the problem of availability of > > > > custom configuration datatypes (DataStorageSpec, DataStorageSpec) for > > > > sub-projects. > > > > > > > > As a result of trying to implement commons-configuration as an > > > > internal API, I have come to the conclusion that the number of changes > > > > and compromises that need to be agreed upon will be very large in this > > > > case. So unless I'm missing something, the proposed design is our > > > > best. > > > > > > > > > > > > Thoughts? > > > > > > > > On Thu, 30 Mar 2023 at 01:42, Maxim Muzafarov <mmu...@apache.org> wrote: > > > > > > > > > > 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<? extends ConfigurationConstraint<?>>[] constraints() default > > > > > {}; > > > > > } > > > > > > > > > > public class NotNullConstraint implements > > > > > ConfigurationConstraint<Object> { > > > > > public void validate(Object newValue) { > > > > > if (newValue == null) > > > > > throw new IllegalArgumentException("Value cannot be > > > > > null"); > > > > > } > > > > > } > > > > > > > > > > public class PositiveConstraint implements > > > > > ConfigurationConstraint<Object> { > > > > > 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 <mmu...@apache.org> > > > > > 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 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 all this is beyond the scope of the > > > > > > current changes. > > > > > > > > > > > > Looking forward to your thoughts. > > > > > > > > > > > > > > > > > > [1] https://lists.apache.org/thread/26j9hhy39okw0wy79mtylb753w6xjclg > > > > > > [2] https://issues.apache.org/jira/browse/CASSANDRA-17734 > > > > > > >
