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