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 <garydgreg...@gmail.com> Sent: Monday, March 3, 2025 2:39 PM To: Apache Logging Developers List <dev@logging.apache.org> 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 <pi...@mailing.copernik.eu> 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 > >
