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; > >> > >> >
