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 <stefan.mikloso...@netapp.com> 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 <bened...@apache.org> > 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 <e.dimitr...@gmail.com> 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 > <mmu...@apache.org<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 > <dcapw...@apple.com<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 > > > <bened...@apache.org<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 > > >> <b.le...@gmail.com<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 > > >> <alek...@apple.com<mailto:alek...@apple.com>> 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 > > >>> <mmu...@apache.org<mailto:mmu...@apache.org>> 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; > > >> > > >> > >
