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