SebTardif commented on PR #4185:
URL: https://github.com/apache/logging-log4j2/pull/4185#issuecomment-5238518413
@ramanathan1504
> One question before I re-approve: `setLayout` returns the string `"Could
not instantiate layout class."` from `invoke()`. A JMX client cannot tell that
apart from a successful result. Should it throw an `MBeanException` instead, or
is returning a diagnostic string the established convention for this MBean?
Happy either way, I just want it to be deliberate.
Good catch. I looked at the package conventions and chose **throw
`MBeanException`** deliberately:
1. `setLayout` is registered as a **void** operation (`MBeanOperationInfo`
return type `"void"`). Success already returns `null`; a free-form error string
is not a reliable client signal.
2. `DynamicMBean.invoke` already declares `throws MBeanException`. That is
the standard JMX way to report an operation failure to the client.
3. The sibling paths mostly use exceptions for invalid API use
(`RuntimeOperationsException` for null attribute/op names,
`ReflectionException` for unknown ops in `HierarchyDynamicMBean`). Informal
strings like `"Options activated."` / `"Hello world."` are success-ish quirks,
not an established failure convention.
4. Server-side `cat.error(...)` is kept so the failure still shows up in
logs.
Pushed in `a7e7964e89`: null instantiate path logs, then `throw new
MBeanException(new IllegalArgumentException(message), message)`. Regression
test now expects `MBeanException` with that target.
Note (out of scope for this PR): `LoggerDynamicMBean.invoke` still returns
`"Hello world."` after a failed `addAppender` instantiate (void helper returns
early). Happy to open a follow-up if you want the same fail-closed JMX signal
there.
Local verification:
```bash
export JAVA_HOME=$(/usr/libexec/java_home -v 17)
./mvnw -pl log4j-1.2-api -am test \
-Dtest=LoggerDynamicMBeanTest,AppenderDynamicMBeanTest \
-Dsurefire.failIfNoSpecifiedTests=false
```
All 4 tests pass. Ready for re-approve when convenient. Thanks again for the
careful review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
