You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/09/11 00:10:01 UTC
[GitHub] [cassandra] dcapwell commented on a change in pull request #749: CASSANDRA-16057: Should update in-jvm dtest to expose stdout and stderr for nodetool
dcapwell commented on a change in pull request #749:
URL: https://github.com/apache/cassandra/pull/749#discussion_r486689934
##########
File path: src/java/org/apache/cassandra/tools/NodeTool.java
##########
@@ -405,10 +415,12 @@ private NodeProbe connect()
nodeClient = nodeProbeFactory.create(host, parseInt(port));
else
nodeClient = nodeProbeFactory.create(host, parseInt(port), username, password);
+
+ nodeClient.setConsoleOutputProvider(consoleOutputProvider);
Review comment:
rather than mutable ref, why not update the create method to take it?
##########
File path: src/java/org/apache/cassandra/tools/NodeProbe.java
##########
@@ -270,6 +272,21 @@ public void close() throws IOException
}
}
+ public void setConsoleOutputProvider(ConsoleOutputProvider consoleOutputProvider)
Review comment:
this makes unit testing nodetool much easier!
##########
File path: src/java/org/apache/cassandra/tools/nodetool/Sjk.java
##########
@@ -143,7 +145,7 @@ else if (isListCommands())
{
for (String cmd : commands.keySet())
{
- System.out.println(String.format("%8s - %s", cmd, parser.getCommandDescription(cmd)));
Review comment:
missing `parser.usage`, you can override it but sadly its annoying.
`parser.usage();` replace with
```
StringBuilder sb = new StringBuilder();
parser.usage(sb);
outStream.println(sb);
```
and `parser.usage(cmd);` replace with
```
StringBuilder sb = new StringBuilder();
parser.usage(cmd, sb);
outStream.println(sb);
```
##########
File path: src/java/org/apache/cassandra/tools/nodetool/Sjk.java
##########
@@ -143,7 +145,7 @@ else if (isListCommands())
{
for (String cmd : commands.keySet())
{
- System.out.println(String.format("%8s - %s", cmd, parser.getCommandDescription(cmd)));
+ outStream.println(String.format("%8s - %s", cmd, parser.getCommandDescription(cmd)));
Review comment:
in the catch block for `CommandAbortedError` there are other logging commands, can replace them as well.
##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -599,20 +602,61 @@ public int liveMemberCount()
public NodeToolResult nodetoolResult(boolean withNotifications, String... commandAndArgs)
{
return sync(() -> {
- DTestNodeTool nodetool = new DTestNodeTool(withNotifications);
- int rc = nodetool.execute(commandAndArgs);
- return new NodeToolResult(commandAndArgs, rc, new ArrayList<>(nodetool.notifications.notifications), nodetool.latestError);
+ try (CapturingConsoleOutputProvider provider = new CapturingConsoleOutputProvider())
+ {
+ DTestNodeTool nodetool = new DTestNodeTool(withNotifications, provider);
+ int rc = nodetool.execute(commandAndArgs);
+ return new NodeToolResult(commandAndArgs, rc,
+ new ArrayList<>(nodetool.notifications.notifications),
+ nodetool.latestError,
+ provider.getOutString(),
+ provider.getErrString());
+ }
}).call();
}
+ private static class CapturingConsoleOutputProvider implements ConsoleOutputProvider
+ {
+ private final ByteArrayOutputStream outBase = new ByteArrayOutputStream();
+ private final ByteArrayOutputStream errBase = new ByteArrayOutputStream();
+ private final PrintStream out = new PrintStream(outBase, true);
+ private final PrintStream err = new PrintStream(errBase, true);
+
+ public PrintStream outStream()
+ {
+ return out;
+ }
+
+ public PrintStream errStream()
+ {
+ return err;
+ }
+
+ public String getOutString()
+ {
+ return outBase.toString();
+ }
+
+ public String getErrString()
+ {
+ return errBase.toString();
+ }
+
+ public void close()
Review comment:
don't need to close `ByteArrayOutputStream`, here is the implementation of close http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/io/ByteArrayOutputStream.java#l267
##########
File path: src/java/org/apache/cassandra/tools/ConsoleOutputProvider.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.io.Closeable;
+import java.io.PrintStream;
+
+public interface ConsoleOutputProvider extends Closeable
Review comment:
why `Closeable `? I don't see anything closing it so this will cause compiler warnings.
##########
File path: test/distributed/org/apache/cassandra/distributed/impl/Instance.java
##########
@@ -599,20 +602,61 @@ public int liveMemberCount()
public NodeToolResult nodetoolResult(boolean withNotifications, String... commandAndArgs)
{
return sync(() -> {
- DTestNodeTool nodetool = new DTestNodeTool(withNotifications);
- int rc = nodetool.execute(commandAndArgs);
- return new NodeToolResult(commandAndArgs, rc, new ArrayList<>(nodetool.notifications.notifications), nodetool.latestError);
+ try (CapturingConsoleOutputProvider provider = new CapturingConsoleOutputProvider())
+ {
+ DTestNodeTool nodetool = new DTestNodeTool(withNotifications, provider);
+ int rc = nodetool.execute(commandAndArgs);
+ return new NodeToolResult(commandAndArgs, rc,
+ new ArrayList<>(nodetool.notifications.notifications),
+ nodetool.latestError,
+ provider.getOutString(),
+ provider.getErrString());
+ }
}).call();
}
+ private static class CapturingConsoleOutputProvider implements ConsoleOutputProvider
+ {
+ private final ByteArrayOutputStream outBase = new ByteArrayOutputStream();
Review comment:
this will cause a compiler warning, will need something like `@SuppressWarnings("resources)` to avoid
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org