Hi Piotr/Gary,
I don't know if this answers the "transaction" question - but a little
input...as the Germans here say "giving my added mustard." 🙂
For my recent changes - adding builders to Filters replacing old factory
methods I have been doing the following.
1.
The single private constructor of a configuration class takes its own Builder
as single argument.
The constructor throws unchecked exceptions on misconfiguration (i.e.
IllegalArgumentException). These should have already been validated in the
builder but there are safety checks - for example, around compiling a Pattern
from a Regex string.
But this constructor is only called by the Builder#build() method (or a
subclass constructor via super).
This also has the advantage of being able to cleanly call 'super(builder)' -
for example, in the Filter implementations to pass the builder up to
AbstractFilter to pick up the onMatch/onMismatch attributes.
There are currently also multiple locations where configuration problems are
not guarded (i.e. Integer/Long from String - the NumberFormatExceptions get
thrown unchecked). For example, XmlConfiguration constructor "shutdownTimeout"
atttribute.
2.
The Builder build() method validates its state - logging errors to the
StatusLogger and returning null on misconfiguration.
The call to the constructor is wrapped in try/catch so that any unchecked
exceptions from #1 are caught and logged - then null is returned.
I have also been incrementally adding JVerify nullability annotations (per
Piotr). If you don't perform the explicit checks the code can light up like a
Christmas tree in the IDE. This happens quite often because the Builder
'@PluginAttribute's are '@Nullable' by design, but they should sometimes be
@NonNull by the time they get to the constructor. 😕 For this reason, the
recently implemented Builder 'isValid()' method doesn't work very well with the
nullability annotations - there is no inferred contract.
One thing to consider (if I haven't interpreted it incorrectly). I believe the
current implementation of a CompositeConfiguration discards a configuration
that fails during reconfiguration (including its configuration-source). So for
example, if a XML configuration does an all-or-nothing approach, or is
temporary unavailable due to a file-lock or a network issue the configuration
is discarded - a subsequent reload has no more information about the failed
configuration - it is simply gone.
Cheers, Jeff
From: Gary Gregory
Sent: Monday, March 3, 2025 2:39 PM
To: Apache Logging Developers List
Subject: Re: Signaling configuration errors
I like the idea of a transactional configuration (all or nothing).
Internally I think we should use a custom unchecked runtime exception which
would be logged by us from our top level API.
Gary
On Mon, Mar 3, 2025, 06:54 Piotr P. Karwasz
wrote:
> Hi all,
>
> Configuration problems in our builders are handled in multiple ways:
>
> 1. Sometimes we call `LOGGER.error` and return `null`.
>
> 2. Sometimes we throw an explicit exception. The types of exceptions
> vary: NPE, IllegalArgumentException, IllegalStateException, etc. There
> is a `o.a.l.l.core.config.ConfigurationException`, but it doesn't seem
> to be used often.
>
> 3. Sometimes exceptions, usually NPEs, are thrown by the constructor of
> a plugin. Most of the time I consider these bugs.
>
> Could we agree on a preferred way to signal configuration errors? I
> don't plan to modify the behavior of old code, but we should apply a
> consistent strategy on new builders.
>
> I do like throwing exceptions, which often simplifies flow control, but
> when I started working on Log4j, I was told 1. was the way to go.
>
> On a related matter, Ralph said on Slack that:
>
> > Failure during configuration should be caught and cause the previous
> configuration to remain intact.
>
> While I agree with this "all-or-nothing" strategy, this is not the way
> it currently works. Regardless of the strategy chosen to communicate the
> problem, a misconfigured component will simply be omitted. This can
> cause problems down the line, like:
>
> 2025-03-03T11:49:26.081922076Z main ERROR RollingFileAppender 'ROLLING':
> When no file name is provided a DirectFileRolloverStrategy must be
> configured
> 2025-03-03T11:49:26.082046726Z main ERROR Null object returned for
> RollingFile in Appenders.
> 2025-03-03T11:49:26.083912635Z main ERROR Unable to locate appender
> "ROLLING" for logger config "root"
>
> However, these problems are not fatal and the configuration replaces the
> previous one.
>
> What do you think?
>
> Piotr
>
>
