simonbence commented on a change in pull request #5356:
URL: https://github.com/apache/nifi/pull/5356#discussion_r712478554
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -216,6 +220,30 @@ public static void main(String[] args) throws IOException,
InterruptedException
dumpFile = null;
verbose = false;
}
+ } else if (cmd.equalsIgnoreCase("status-history")) {
+ if (args.length != 4) {
+ System.err.printf("Wrong number of arguments: %d instead of 4,
the command parameters are: " +
+ "status-history --days <number of days> <dumpFile>%n",
args.length);
+ System.exit(0);
+ }
+ statusHistoryDays = args[2];
+ try {
+ final int numberOfDays = Integer.parseInt(statusHistoryDays);
+ if (numberOfDays < 1) {
+ System.err.println("The number of days must be positive
and greater than zero.");
+ System.exit(0);
Review comment:
Exit 0 is for cases where there is no issue. It would be better to use
some error codes
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -175,7 +177,8 @@ private static void printUsage() {
System.out.println("Status : Determine if there is a running instance
of Apache NiFi");
System.out.println("Dump : Write a Thread Dump to the file specified
by [options], or to the log if no file is given");
System.out.println("Diagnostics : Write diagnostic information to the
file specified by [options], or to the log if no file is given. The --verbose
flag may be provided as an option before " +
- "the filename, which may result in additional diagnostic
information being written.");
+ "the filename, which may result in additional diagnostic
information being written.");
+ System.out.println("Status-history : Save the status history to the
file specified by [options], or to the bootstrap log if no file is given");
Review comment:
I would still think that describing the expected arguments up front (not
after the first failure) would be useful
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -216,6 +220,30 @@ public static void main(String[] args) throws IOException,
InterruptedException
dumpFile = null;
verbose = false;
}
+ } else if (cmd.equalsIgnoreCase("status-history")) {
+ if (args.length != 4) {
+ System.err.printf("Wrong number of arguments: %d instead of 4,
the command parameters are: " +
+ "status-history --days <number of days> <dumpFile>%n",
args.length);
Review comment:
It might be my perfonal preference, but the "--days" argument does not
provide any information, thus it can be removed. I think, maybe the best would
be to remove that and swap the two remaining arguments. By this, we can offer a
default for days (1? 2?) and allow users privde the file only
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/NodeStatusHistoryDump.java
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import com.fasterxml.jackson.core.util.DefaultIndenter;
+import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.nifi.web.api.dto.status.StatusHistoryDTO;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+public class NodeStatusHistoryDump implements StatusHistoryDump {
+
+ private final StatusHistory statusHistory;
+
+ public NodeStatusHistoryDump(final StatusHistory statusHistory) {
+ this.statusHistory = statusHistory;
+ }
+
+ @Override
+ public void writeTo(final OutputStream out) throws IOException {
+ final ObjectMapper objectMapper = new ObjectMapper();
+ final DefaultPrettyPrinter prettyPrinter = new DefaultPrettyPrinter();
Review comment:
It would possibly makes sense to provide some customability to the
format.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/scheduling/ProcessorLifecycleIT.java
##########
@@ -557,10 +558,12 @@ private FlowManagerAndSystemBundle
buildFlowControllerForTest(final String propK
final ExtensionDiscoveringManager extensionManager = new
StandardExtensionDiscoveringManager();
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ final StatusHistoryRepository statusHistoryRepository =
mock(StatusHistoryRepository.class);
Review comment:
Why not on-the-fly mocking (like the other ones)?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/NodeStatusHistoryDumpFactory.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import java.util.Calendar;
+import java.util.Date;
+
+public class NodeStatusHistoryDumpFactory implements StatusHistoryDumpFactory {
+
+ private StatusHistoryRepository statusHistoryRepository;
+
+ @Override
+ public StatusHistoryDump create(int days) {
+ final Date now = new Date();
Review comment:
I am not quite sure if the date calculation is the best from user
perspective. Would not it make more sense to use calendar day? From user
perspective it looks to be simpler to ask for the data for this day and the day
before for example (numberOfDays is 2)
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/scheduling/ProcessorLifecycleIT.java
##########
@@ -614,7 +619,7 @@ public Bundle getSystemBundle() {
private final List<String> operationNames = new LinkedList<>();
void setScenario(Runnable onScheduleCallback, Runnable
onUnscheduleCallback, Runnable onStopCallback,
- Runnable onTriggerCallback) {
+ Runnable onTriggerCallback) {
Review comment:
Minor: I think this is not the indentation of arguments we generally use
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryDump.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+/**
+ * Interface to serialize status history dumps.
+ */
+public interface StatusHistoryDump {
+
+ /**
+ * Writes a status history dump to an output stream.
+ *
+ * @param out - the output stream
Review comment:
I am not sure, how this affect the generated doc, but using "-" seems
pretty unusual
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/NodeStatusHistoryDump.java
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import com.fasterxml.jackson.core.util.DefaultIndenter;
+import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.nifi.web.api.dto.status.StatusHistoryDTO;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+public class NodeStatusHistoryDump implements StatusHistoryDump {
Review comment:
The name should refer to the JSON output. This implementation is defined
by providing JSON results.
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryDumpFactory.java
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.nifi.controller.status.history;
+
+/**
+ * Interface to create status history dumps.
+ */
+public interface StatusHistoryDumpFactory {
+
+ /**
+ * Creates a status history dump.
+ *
+ * @param days - number of backdating days
Review comment:
The "-" as well
##########
File path:
nifi-framework-api/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryDump.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import java.io.IOException;
+import java.io.OutputStream;
+
+/**
+ * Interface to serialize status history dumps.
Review comment:
I would suggest to rephrase this. Something like "Container for status
history data which is capable to write it in an implementation dependent
format" would be more describing
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/NodeStatusHistoryDumpFactory.java
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.nifi.controller.status.history;
+
+import java.util.Calendar;
+import java.util.Date;
+
+public class NodeStatusHistoryDumpFactory implements StatusHistoryDumpFactory {
Review comment:
The same naming question as in case of NodeStatusHistoryDump
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -729,6 +761,17 @@ public void dump(final File dumpFile) throws IOException {
makeRequest(DUMP_CMD, null, dumpFile, "thread dump");
}
+ /**
+ * Writes NiFi status history information to the given file; if file is
null, logs at
+ * INFO level instead.
+ *
+ * @param dumpFile the file to write the dump content to
+ * @throws IOException if any issues occur while writing the dump file
+ */
+ public void statusHistory(final File dumpFile, final String days) throws
IOException {
Review comment:
I think something is not quiet right. In line 246, a value is assigned
to `dumpFile` explicity. (Which cannot contain null, neither because of the
argument check or the `File` class's NPE check). In the other hand, the
documentation of the `statusHistory` says, "if file is null", which because of
this looks like an unreachable state.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/TestFlowController.java
##########
@@ -117,12 +117,13 @@
private BulletinRepository bulletinRepo;
private VariableRegistry variableRegistry;
private ExtensionDiscoveringManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
Review comment:
Not necessary to make it in instance level attribute, as it is used only
once
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/StandardProcessorNodeIT.java
##########
@@ -103,6 +104,7 @@
private Bundle systemBundle;
private ExtensionDiscoveringManager extensionManager;
private NiFiProperties niFiProperties;
+ private StatusHistoryRepository statusHistoryRepository;
Review comment:
Not necessary to make it in instance level attribute, as it is used only
once
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/StandardFlowServiceTest.java
##########
@@ -91,8 +94,9 @@ public void setup() throws Exception {
revisionManager = mock(RevisionManager.class);
extensionManager = mock(ExtensionDiscoveringManager.class);
flowController =
FlowController.createStandaloneInstance(mockFlowFileEventRepository,
properties, authorizer, mockAuditService, mockEncryptor,
- new VolatileBulletinRepository(),
variableRegistry, mock(FlowRegistryClient.class), extensionManager);
+ new VolatileBulletinRepository(),
variableRegistry, mock(FlowRegistryClient.class), extensionManager,
statusHistoryRepository);
flowService =
StandardFlowService.createStandaloneInstance(flowController, properties,
mockEncryptor, revisionManager, authorizer);
+ statusHistoryRepository = new VolatileComponentStatusRepository();
Review comment:
Please use mock instead of instance
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/serialization/StandardFlowSerializerTest.java
##########
@@ -89,12 +92,14 @@ public void setUp() throws Exception {
extensionManager = new StandardExtensionDiscoveringManager();
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ statusHistoryRepository = new VolatileComponentStatusRepository();
Review comment:
Why not mocking?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-headless-server/src/main/java/org/apache/nifi/headless/HeadlessNiFiServer.java
##########
@@ -137,7 +141,8 @@ public void preDestruction() throws
AuthorizerDestructionException {
bulletinRepository,
variableRegistry,
flowRegistryClient,
- extensionManager);
+ extensionManager,
+ statusHistoryRepository);
Review comment:
This is not something we want to introduce with this change. By this
change status history would be involved into headless NiFi (also with hard
coded implementation) which might come with unforeseen consequences. Please
find a solution which does not include changing headless.
--
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]
