utafrali commented on code in PR #2720:
URL: https://github.com/apache/activemq/pull/2720#discussion_r4336604126
##########
activemq-console/src/main/java/org/apache/activemq/console/command/AbstractJmxCommand.java:
##########
@@ -388,7 +388,7 @@ public void execute(List<String> tokens) throws Exception {
try {
super.execute(tokens);
} catch (Exception exception) {
- handleException(exception, jmxServiceUrl.toString());
+ handleException(exception, jmxServiceUrl != null ?
jmxServiceUrl.toString() : "local");
Review Comment:
The fallback `"local"` works, but the resulting message will read `"Broker
not available at: local"` which is a bit ambiguous. Since `isJmxUseLocal()` is
available on `this`, a slightly clearer fallback would be `isJmxUseLocal() ?
"local JVM MBean server" : "(unknown)"`. Minor, but easier to parse in a log.
##########
activemq-console/src/main/java/org/apache/activemq/console/command/AbstractCommand.java:
##########
@@ -162,13 +162,9 @@ protected void printHelpFromFile() {
}
protected void handleException(Exception exception, String serviceUrl)
throws Exception {
Review Comment:
No regression test was added. Both NPE paths (exception with no cause, and
null `jmxServiceUrl`) are straightforward to unit-test. A simple test that
calls `handleException` with a plain `new RuntimeException("msg")` (no cause)
and verifies either the rethrow or the `ConnectException` branch would prevent
these from silently regressing.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
