PakhomovAlexander commented on code in PR #1249:
URL: https://github.com/apache/ignite-3/pull/1249#discussion_r1005304923
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/call/CallExecutionPipeline.java:
##########
@@ -83,6 +90,19 @@ public static <I extends CallInput, T>
CallExecutionPipelineBuilder<I, T> builde
* @return exit code.
*/
public int runPipeline() {
+ try {
+ if (verbose) {
+ CliLoggers.redirectOutput(errOutput);
+ }
+ return runPipelineInternal();
+ } finally {
+ if (verbose) {
+ CliLoggers.redirectOutput(null);
Review 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()`.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/flow/builder/FlowBuilderImpl.java:
##########
@@ -46,9 +47,10 @@
private final Flow<I, O> flow;
private final ExceptionHandlers exceptionHandlers;
private final DecoratorRegistry decoratorRegistry;
+ private boolean verbose;
Review Comment:
why not final?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/logger/CliLoggers.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.ignite.internal.cli.logger;
+
+import java.io.PrintWriter;
+import java.lang.System.Logger;
+import java.util.ResourceBundle;
+import okhttp3.OkHttpClient;
+import okhttp3.OkHttpClient.Builder;
+import okhttp3.logging.HttpLoggingInterceptor;
+import okhttp3.logging.HttpLoggingInterceptor.Level;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.lang.LoggerFactory;
+import org.apache.ignite.rest.client.invoker.ApiClient;
+import org.apache.ignite.rest.client.invoker.Configuration;
+
+public class CliLoggers {
+ private static PrintWriter output;
+
+ private static boolean isVerbose;
+
+ private static final LoggerFactory loggerFactory = name -> new
CliLogger(System.getLogger(name));
+
+ public static IgniteLogger forClass(Class<?> cls) {
+ return Loggers.forClass(cls, loggerFactory);
+ }
+
+ public static IgniteLogger forName(String name) {
+ return Loggers.forName(name, loggerFactory);
+ }
+
+ public static void redirectOutput(PrintWriter output) {
+ CliLoggers.output = output;
+ boolean isVerbose = output != null;
+ if (CliLoggers.isVerbose != isVerbose) {
+ setHttpLogging(isVerbose);
+ }
+ CliLoggers.isVerbose = isVerbose;
+ }
+
+ private static void setHttpLogging(boolean isVerbose) {
Review 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.
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/flow/FlowTest.java:
##########
@@ -53,6 +55,25 @@ class FlowTest {
private StringWriter out;
private StringWriter errOut;
+ @Test
+ void testVerbose() throws IOException {
Review 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.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/logger/CliLoggers.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.ignite.internal.cli.logger;
+
+import java.io.PrintWriter;
+import java.lang.System.Logger;
+import java.util.ResourceBundle;
+import okhttp3.OkHttpClient;
+import okhttp3.OkHttpClient.Builder;
+import okhttp3.logging.HttpLoggingInterceptor;
+import okhttp3.logging.HttpLoggingInterceptor.Level;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.lang.LoggerFactory;
+import org.apache.ignite.rest.client.invoker.ApiClient;
+import org.apache.ignite.rest.client.invoker.Configuration;
+
+public class CliLoggers {
Review Comment:
It might be useful to add a javadoc that explains why this class exists and
when to use it.
--
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]
