diff --git a/src/java/org/apache/cassandra/tools/NodeTool.java b/src/java/org/apache/cassandra/tools/NodeTool.java index b51ffc013c..884e783a6b 100644 --- a/src/java/org/apache/cassandra/tools/NodeTool.java +++ b/src/java/org/apache/cassandra/tools/NodeTool.java @@ -124,7 +124,7 @@ public class NodeTool .findFirst().orElse(null); if (connectFailure != null) { - output.out.println("nodetool: " + connectFailure.getMessage()); + output.err.println("nodetool: " + connectFailure.getMessage()); return 1; } diff --git a/src/java/org/apache/cassandra/tools/RemoteJmxMBeanAccessor.java b/src/java/org/apache/cassandra/tools/RemoteJmxMBeanAccessor.java index 6891b85fdb..00f9c2e796 100644 --- a/src/java/org/apache/cassandra/tools/RemoteJmxMBeanAccessor.java +++ b/src/java/org/apache/cassandra/tools/RemoteJmxMBeanAccessor.java @@ -247,7 +247,7 @@ public class RemoteJmxMBeanAccessor implements MBeanAccessor close(); throw new RuntimeException("Invalid ObjectName? Please report this as a bug.", e); } - catch (IOException e) + catch (IOException | SecurityException e) { close(); Throwable rootCause = Throwables.getRootCause(e); diff --git a/src/java/org/apache/cassandra/tools/nodetool/JmxConnect.java b/src/java/org/apache/cassandra/tools/nodetool/JmxConnect.java index 4a5c02d6bc..e010a61719 100644 --- a/src/java/org/apache/cassandra/tools/nodetool/JmxConnect.java +++ b/src/java/org/apache/cassandra/tools/nodetool/JmxConnect.java @@ -31,9 +31,9 @@ import com.google.common.base.Throwables; import org.apache.cassandra.io.util.File; import org.apache.cassandra.tools.INodeProbeFactory; import org.apache.cassandra.tools.NodeProbe; +import org.apache.cassandra.tools.nodetool.strategy.NodetoolConnectionException; import picocli.CommandLine.Command; -import picocli.CommandLine.InitializationException; import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Option; import picocli.CommandLine.Spec; @@ -96,9 +96,11 @@ public class JmxConnect extends AbstractCommand implements AutoCloseable catch (IOException | SecurityException e) { Throwable rootCause = Throwables.getRootCause(e); - output.printError("nodetool: Failed to connect to '%s:%s' - %s: '%s'.%n", host, port, - rootCause.getClass().getSimpleName(), rootCause.getMessage()); - throw new InitializationException("Failed to connect to JMX", e); + throw new NodetoolConnectionException(String.format("Failed to connect to '%s:%s' - %s: '%s'.", + host, port, + rootCause.getClass().getSimpleName(), + rootCause.getMessage()), + e); } } diff --git a/src/java/org/apache/cassandra/tools/nodetool/strategy/CommandMBeanExecutionStrategy.java b/src/java/org/apache/cassandra/tools/nodetool/strategy/CommandMBeanExecutionStrategy.java index f3fbb9a2c8..e9243ae763 100644 --- a/src/java/org/apache/cassandra/tools/nodetool/strategy/CommandMBeanExecutionStrategy.java +++ b/src/java/org/apache/cassandra/tools/nodetool/strategy/CommandMBeanExecutionStrategy.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.util.List; import java.util.Map; -import javax.management.InstanceNotFoundException; import javax.management.MBeanException; import javax.management.MBeanServerConnection; import javax.management.ObjectName; @@ -91,8 +90,12 @@ public class CommandMBeanExecutionStrategy implements CommandExecutionStrategy try { - String rawResult = executeViaRemoteMBean(((RemoteJmxMBeanAccessor) probe.getMBeanAccessor()).getMBeanServerConnection(), - commandName, jsonParams); + MBeanServerConnection mbs = ((RemoteJmxMBeanAccessor) probe.getMBeanAccessor()).getMBeanServerConnection(); + + if (!mbs.isRegistered(constructCommandMBeanName(commandName))) + return new CommandLine.RunLast().execute(parseResult); + + String rawResult = executeViaRemoteMBean(mbs, commandName, jsonParams); Map commandResult = fromJsonMap(rawResult); @@ -179,9 +182,6 @@ public class CommandMBeanExecutionStrategy implements CommandExecutionStrategy { ObjectName mbeanName = constructCommandMBeanName(commandName); - if (!mbs.isRegistered(mbeanName)) - throw new InstanceNotFoundException("Command MBean not found: " + mbeanName); - try { Object result = mbs.invoke(mbeanName, CommandMBeanAdapter.INVOKE_METHOD, diff --git a/src/java/org/apache/cassandra/tools/nodetool/strategy/ProtocolAwareExecutionStrategy.java b/src/java/org/apache/cassandra/tools/nodetool/strategy/ProtocolAwareExecutionStrategy.java index 9d384b58c7..e912e026fd 100644 --- a/src/java/org/apache/cassandra/tools/nodetool/strategy/ProtocolAwareExecutionStrategy.java +++ b/src/java/org/apache/cassandra/tools/nodetool/strategy/ProtocolAwareExecutionStrategy.java @@ -61,10 +61,10 @@ public class ProtocolAwareExecutionStrategy implements CommandExecutionStrategy } catch (ExecutionStrategyCloseException e) { - connectCmd.commandLine() - .getErr() - .println("Failed to connect: " + e.getMessage()); - return connectCmd.commandLine().getExitCodeExceptionMapper().getExitCode(e); + CommandLine commandLine = parseResult.commandSpec().commandLine(); + commandLine.getErr().println("Failed to close connection: " + e.getMessage()); + CommandLine.IExitCodeExceptionMapper mapper = commandLine.getExitCodeExceptionMapper(); + return mapper != null ? mapper.getExitCode(e) : commandLine.getCommandSpec().exitCodeOnExecutionException(); } } diff --git a/test/unit/org/apache/cassandra/tools/NodeProbeMBeanProxyTest.java b/test/unit/org/apache/cassandra/tools/NodeProbeMBeanProxyTest.java new file mode 100644 index 0000000000..acc9da1c79 --- /dev/null +++ b/test/unit/org/apache/cassandra/tools/NodeProbeMBeanProxyTest.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.tools; + +import java.util.List; +import java.util.Map; + +import javax.management.InstanceNotFoundException; + +import org.junit.Test; + +import org.apache.cassandra.db.ColumnFamilyStoreMBean; +import org.apache.cassandra.db.compression.CompressionDictionaryManagerMBean; +import org.apache.cassandra.management.MBeanAccessor; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class NodeProbeMBeanProxyTest +{ + @Test + public void testMissingMBeanSurfacesAsInstanceNotFound() + { + NodeProbe probe = new NodeProbe(new NullMBeanAccessor()); + + assertThatThrownBy(probe::stopCassandraDaemon) + .isInstanceOf(RuntimeException.class) + .hasRootCauseInstanceOf(InstanceNotFoundException.class) + .hasRootCauseMessage("StorageServiceMBean is not available on this node"); + } + + private static class NullMBeanAccessor implements MBeanAccessor + { + @Override + public T findMBean(Class clazz) + { + return null; + } + + @Override + public T findMBeanMetric(Class clazz, Props props) + { + return null; + } + + @Override + public boolean isMBeanMetricRegistered(Props props) + { + return false; + } + + @Override + public ColumnFamilyStoreMBean findColumnFamily(String type, String keyspace, String columnFamily) + { + return null; + } + + @Override + public CompressionDictionaryManagerMBean findCompressionDictionary(String keyspace, String table) + { + return null; + } + + @Override + public List threadPoolInfos() + { + return List.of(); + } + + @Override + public List> findColumnFamilies(String type) + { + return List.of(); + } + } +} diff --git a/test/unit/org/apache/cassandra/tools/nodetool/NodetoolConnectFailureTest.java b/test/unit/org/apache/cassandra/tools/nodetool/NodetoolConnectFailureTest.java index 7c0d843f1a..9c1f772f44 100644 --- a/test/unit/org/apache/cassandra/tools/nodetool/NodetoolConnectFailureTest.java +++ b/test/unit/org/apache/cassandra/tools/nodetool/NodetoolConnectFailureTest.java @@ -46,17 +46,34 @@ public class NodetoolConnectFailureTest extends CQLTester @Test public void cqlConnectFailurePrintsFriendlyMessageAndExits() + { + assertConnectFailure("cql", "nodetool: Failed to connect to '" + CLOSED_HOST + ':' + CLOSED_PORT + "' via CQL"); + } + + @Test + public void staticMBeanConnectFailurePrintsFriendlyMessageAndExits() + { + assertConnectFailure("static_mbean", "nodetool: Failed to connect to '" + CLOSED_HOST + ':' + CLOSED_PORT + '\''); + } + + @Test + public void commandMBeanConnectFailurePrintsFriendlyMessageAndExits() + { + assertConnectFailure("command_mbean", "nodetool: Failed to connect to '" + CLOSED_HOST + ':' + CLOSED_PORT + '\''); + } + + private static void assertConnectFailure(String protocol, String expectedMessage) { try (WithProperties ignored = new WithProperties() - .set(CassandraRelevantProperties.CASSANDRA_CLI_EXECUTION_PROTOCOL, "cql")) + .set(CassandraRelevantProperties.CASSANDRA_CLI_EXECUTION_PROTOCOL, protocol)) { ToolRunner.ToolResult result = ToolRunner.invokeNodetoolInJvm(NodeTool::new, NodetoolConnectFailureTest::closedTargetArgs, "status"); - assertThat(result.getExitCode()).as("CQL connect failure should exit 1 (clean), not 2 (stack trace)") + assertThat(result.getExitCode()).as("connect failure should exit 1 (clean), not 2 (stack trace)") .isEqualTo(1); - assertThat(result.getStdout()).contains("nodetool: Failed to connect to '" + CLOSED_HOST + ':' + CLOSED_PORT + "' via CQL"); - assertThat(result.getCleanedStderr()).doesNotContain("-- StackTrace --"); + assertThat(result.getCleanedStderr()).contains(expectedMessage) + .doesNotContain("-- StackTrace --"); } }