IGNITE-17786 Add --verbose option to all commands #1249
Conversation
| return runPipelineInternal(); | ||
| } finally { | ||
| if (verbose) { | ||
| CliLoggers.redirectOutput(null); |
There was a problem hiding this comment.
CliLoggers.redirectOutput(null); looks strange. It could be read as "redirect logger output to nothing" that might give a false illustion that the logger won't write logs at all. But in fact, it writes to log file. I would suggest to add one more method like setDefaultOutput().
| private final Flow<I, O> flow; | ||
| private final ExceptionHandlers exceptionHandlers; | ||
| private final DecoratorRegistry decoratorRegistry; | ||
| private boolean verbose; |
There was a problem hiding this comment.
It's not final because it's set by the method, not the constructor.
| import org.apache.ignite.rest.client.invoker.ApiClient; | ||
| import org.apache.ignite.rest.client.invoker.Configuration; | ||
|
|
||
| public class CliLoggers { |
There was a problem hiding this comment.
It might be useful to add a javadoc that explains why this class exists and when to use it.
| CliLoggers.isVerbose = isVerbose; | ||
| } | ||
|
|
||
| private static void setHttpLogging(boolean isVerbose) { |
There was a problem hiding this comment.
I think this code should not be located in a class named CliLoggers. We have to rename this class or move this http-related code to a separate class.
| private StringWriter errOut; | ||
|
|
||
| @Test | ||
| void testVerbose() throws IOException { |
There was a problem hiding this comment.
this test is good and it would be nice to test CallExcecutionPipeline too. Maybe it could be just a regular command test but with --verbose flag.
valepakh
force-pushed
the
IGNITE-17786
branch
from
2b034fb to
ea78aa4
Compare
|
LGTM |
| protected boolean usageHelpRequested; | ||
|
|
||
| @Option(names = {"-v", "--verbose"}, description = "Show additional information about errors.") | ||
| protected boolean verbose; |
There was a problem hiding this comment.
Is it always about errors?
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/flow/builder/FlowBuilder.java
Outdated
Show resolved
Hide resolved
| ApiClient client = Configuration.getDefaultApiClient(); | ||
| Builder builder = client.getHttpClient().newBuilder(); | ||
|
|
||
| builder.interceptors().clear(); |
There was a problem hiding this comment.
Won't it clear all interceptors when we only want to stop http log?
Also, adding a test checking correct behaviour of this method would be cool
…flow/builder/FlowBuilder.java Co-authored-by: Semyon Danilov <samvimes@yandex.ru>
3 participants
https://issues.apache.org/jira/browse/IGNITE-17786