simonbence commented on a change in pull request #5356:
URL: https://github.com/apache/nifi/pull/5356#discussion_r702859141
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -216,6 +219,9 @@ public static void main(String[] args) throws IOException,
InterruptedException
dumpFile = null;
verbose = false;
}
+ } else if (cmd.equalsIgnoreCase("status-history")) {
+ statusHistoryDays = args[2];
Review comment:
I think a check on the number of arguments (like in `dump` or
`diagnostic`) would be good here. If not for else, but line 224 will end up in
a NPE if the args[3] is null (and `ArrayOutOfBounds` if not enough args).
Anyway, some "guarding" would be needed here.
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -275,6 +282,9 @@ public static void main(String[] args) throws IOException,
InterruptedException
case "env":
runNiFi.env();
break;
+ case "status-history":
Review comment:
Minor: just by the nature of the things, it migh make sense to move this
just below the `diagnostic`
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -185,13 +187,14 @@ private static void printUsage() {
}
public static void main(String[] args) throws IOException,
InterruptedException {
- if (args.length < 1 || args.length > 3) {
+ if (args.length < 1 || args.length > 4) {
printUsage();
return;
}
File dumpFile = null;
boolean verbose = false;
+ String statusHistoryDays = null;
Review comment:
I am not quite sure about how and when this is being parsed but I am not
fund of the idea that a value which has a name suggests to be a number (days)
is handled as string. This should be a number. Integer let's say and also
checked to be a positive number, bigger than zero. Depending on your purposes,
this might give you the opportunity to provide a default value as well.
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -729,6 +739,15 @@ public void dump(final File dumpFile) throws IOException {
makeRequest(DUMP_CMD, null, dumpFile, "thread dump");
}
+ /**
Review comment:
If I understand it well, based on the `if` at the beginning of the
`main` method, the file cannot be null. Also: I would find it more readable to
use the usual JavaDoc format and handle arguments in their separate block.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/integration/Cluster.java
##########
@@ -141,8 +143,8 @@ public Node createNode() {
final ExtensionDiscoveringManager extensionManager = new
StandardExtensionDiscoveringManager();
final FingerprintFactory fingerprintFactory = new
FingerprintFactory(encryptor, extensionManager, sensitiveValueEncoder);
final FlowElection flowElection = new
PopularVoteFlowElection(flowElectionTimeoutMillis, TimeUnit.MILLISECONDS,
flowElectionMaxNodes, fingerprintFactory);
-
- final Node node = new Node(nifiProperties, extensionManager,
flowElection);
+ final StatusHistoryRepository statusHistoryRepository = new
VolatileComponentStatusRepository();
Review comment:
Minor: as `VolatileComponentStatusRepository` has a constructor with
`NiFiProperties` as argument (which you have here) it would might worth to call
that constructor instead of the no-arg one
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -119,6 +119,7 @@
public static final String DUMP_CMD = "DUMP";
public static final String DIAGNOSTICS_CMD = "DIAGNOSTICS";
public static final String IS_LOADED_CMD = "IS_LOADED";
+ public static final String STATUS_HISTORY_CMD = "STATUS_HISTORY";
Review comment:
Next time please squash commits before publishing. Thanks!
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/FlowController.java
##########
@@ -1172,22 +1175,6 @@ private ProvenanceRepository
createProvenanceRepository(final NiFiProperties pro
}
}
- private StatusHistoryRepository createStatusHistoryRepository() {
Review comment:
Nice work, thanks for extracting this!
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/spring/StatusHistoryRepositoryFactoryBean.java
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.spring;
+
+import org.apache.nifi.controller.status.history.StatusHistoryRepository;
+import org.apache.nifi.nar.ExtensionManager;
+import org.apache.nifi.nar.NarThreadContextClassLoader;
+import org.apache.nifi.util.NiFiProperties;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+public class StatusHistoryRepositoryFactoryBean implements
FactoryBean<StatusHistoryRepository>, ApplicationContextAware {
+
+ private static final String DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION =
"org.apache.nifi.controller.status.history.VolatileComponentStatusRepository";
+
+ private ApplicationContext applicationContext;
+ private NiFiProperties nifiProperties;
+ private ExtensionManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
+
+ @Override
+ public StatusHistoryRepository getObject() throws Exception {
+ nifiProperties = applicationContext.getBean("nifiProperties",
NiFiProperties.class);
+ extensionManager = applicationContext.getBean("extensionManager",
ExtensionManager.class);
+ final String implementationClassName =
nifiProperties.getProperty(NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION,
DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION);
+ if (implementationClassName == null) {
+ throw new RuntimeException("Cannot create Status History
Repository because the NiFi Properties is missing the following property: "
+ +
NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION);
+ }
+
+ try {
+ statusHistoryRepository =
NarThreadContextClassLoader.createInstance(extensionManager,
implementationClassName, StatusHistoryRepository.class, nifiProperties);
+ statusHistoryRepository.start();
Review comment:
Minor: if we want to go with the Spring-wibe I suggeset to use the
`start` method as an _init_ method instead of calling it here.
[Example](https://mkyong.com/spring/spring-init-method-and-destroy-method-example/)
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/spring/StatusHistoryRepositoryFactoryBean.java
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.spring;
+
+import org.apache.nifi.controller.status.history.StatusHistoryRepository;
+import org.apache.nifi.nar.ExtensionManager;
+import org.apache.nifi.nar.NarThreadContextClassLoader;
+import org.apache.nifi.util.NiFiProperties;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+public class StatusHistoryRepositoryFactoryBean implements
FactoryBean<StatusHistoryRepository>, ApplicationContextAware {
+
+ private static final String DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION =
"org.apache.nifi.controller.status.history.VolatileComponentStatusRepository";
+
+ private ApplicationContext applicationContext;
+ private NiFiProperties nifiProperties;
+ private ExtensionManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
+
+ @Override
+ public StatusHistoryRepository getObject() throws Exception {
+ nifiProperties = applicationContext.getBean("nifiProperties",
NiFiProperties.class);
+ extensionManager = applicationContext.getBean("extensionManager",
ExtensionManager.class);
+ final String implementationClassName =
nifiProperties.getProperty(NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION,
DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION);
+ if (implementationClassName == null) {
+ throw new RuntimeException("Cannot create Status History
Repository because the NiFi Properties is missing the following property: "
Review comment:
As now this is a Spring-specific code it might worth to check if
throwing `BeanCreationException` would be better.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/spring/StatusHistoryRepositoryFactoryBean.java
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.spring;
+
+import org.apache.nifi.controller.status.history.StatusHistoryRepository;
+import org.apache.nifi.nar.ExtensionManager;
+import org.apache.nifi.nar.NarThreadContextClassLoader;
+import org.apache.nifi.util.NiFiProperties;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+public class StatusHistoryRepositoryFactoryBean implements
FactoryBean<StatusHistoryRepository>, ApplicationContextAware {
Review comment:
Please add some description here
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/reporting/TestStandardReportingContext.java
##########
@@ -87,6 +90,8 @@ public void setup() {
extensionManager = new StandardExtensionDiscoveringManager();
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ statusHistoryRepository = new VolatileComponentStatusRepository();
Review comment:
Same as in case of `TestFlowController`
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/TestFlowController.java
##########
@@ -136,6 +139,8 @@ public void setup() {
extensionManager = new StandardExtensionDiscoveringManager();
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ statusHistoryRepository = new VolatileComponentStatusRepository();
Review comment:
As you have NiFi properties here, it is suggested to use the relevant
constructor. It might be even better to use a mock, as in case of
`flowFileEventRepo` or `auditService`.
##########
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 +559,12 @@ private FlowManagerAndSystemBundle
buildFlowControllerForTest(final String propK
final ExtensionDiscoveringManager extensionManager = new
StandardExtensionDiscoveringManager();
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ final StatusHistoryRepository statusHistoryRepository = new
VolatileComponentStatusRepository();
Review comment:
Same as in case of `TestFlowController`
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/integration/FrameworkIntegrationTest.java
##########
@@ -231,6 +232,8 @@ protected final void initialize(final NiFiProperties
nifiProperties) throws IOEx
systemBundle = SystemBundle.create(nifiProperties);
extensionManager.discoverExtensions(systemBundle,
Collections.emptySet());
+ statusHistoryRepository = new VolatileComponentStatusRepository();
Review comment:
Same as in case of `TestFlowController`
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -175,7 +176,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 log if no file is given");
Review comment:
Please specify, which log (bootstrap)
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-runtime/src/main/java/org/apache/nifi/BootstrapListener.java
##########
@@ -244,6 +245,12 @@ public void run() {
sendAnswer(socket.getOutputStream(),
answer);
logger.debug("Responded to IS_LOADED
request from Bootstrap with value: " + answer);
break;
+ case STATUS_HISTORY:
Review comment:
Minor: I know that the order of the commands is not kept consistently
everywhere but in general I think we should try to keep it as much as possible.
So I would suggest to move this just right after diagnostic.
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -185,13 +187,14 @@ private static void printUsage() {
}
public static void main(String[] args) throws IOException,
InterruptedException {
- if (args.length < 1 || args.length > 3) {
+ if (args.length < 1 || args.length > 4) {
Review comment:
If my understanding is correct and the `status-history` supports two
arguments, this should not be changed
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1263,6 +1276,29 @@ public DecommissionTask getDecommissionTask() {
return decommissionTask;
}
+ @Override
+ public String getNodeStatusHistoryJson(final int days) {
+ final Date now = new Date();
+ final Calendar calendar = Calendar.getInstance();
+ calendar.setTime(now);
+ calendar.add(Calendar.DATE, days);
+ final Date daysBefore = calendar.getTime();
+ final StatusHistory nodeStatusHistory =
statusHistoryRepository.getNodeStatusHistory(daysBefore, now);
+
+ final ObjectMapper objectMapper = new ObjectMapper();
+ final DefaultPrettyPrinter prettyPrinter = new DefaultPrettyPrinter();
+
prettyPrinter.indentArraysWith(DefaultIndenter.SYSTEM_LINEFEED_INSTANCE);
+
+ final StatusHistoryDTO statusHistoryDTO =
StatusHistoryUtil.createStatusHistoryDTO(nodeStatusHistory);
+ try {
+ return
objectMapper.writer(prettyPrinter).writeValueAsString(statusHistoryDTO);
Review comment:
Moving the whole dataset (possibly multiple days of verbose JSON data)
into a String looks like a kind of waste. If this method ends up returning some
kind of marshalled data instead of a `StatusHistoryDTO`, it would be preferred
to use the `writer.writeValue(OutputStream, Object)` method instead and
returning with the `OutputStream`
##########
File path: nifi-server-api/src/main/java/org/apache/nifi/NiFiServer.java
##########
@@ -40,4 +40,6 @@
DiagnosticsFactory getThreadDumpFactory();
DecommissionTask getDecommissionTask();
+
+ String getNodeStatusHistoryJson(final int days);
Review comment:
I find this method alien to this contract. This is a foundational
contract of the NiFi and should be as narrow as it is possible. I am not sure
about what would be the exact solution, but I see two opportunities. Ordered
based on my personal preference:
1./
Use the "Interface Segregation Principle" and create a new (partial)
interface which would have this new method. Something like `StatusProvider`
(not necessary the perfect name, but I would not go neither too generic or
narrow). Relevant classes might implement this interface as well (The best
would be if only `JettyServer` would need to do this, not `HeadlessNiFiServer`
or `MiNiFiServer`) the same way as now. The given instance should be exposed to
the client (`BootstrapListener`) via this interface.
2./
In case the one above is not reachable, I would suggest to hook on the
`getDiagnosticsFactory` .
Anyway, the main point would be to not dilute the core interface and expose
as less as it is possible.
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -275,6 +282,9 @@ public static void main(String[] args) throws IOException,
InterruptedException
case "env":
runNiFi.env();
break;
+ case "status-history":
Review comment:
- Calling with no arguments results `ArrayIndexOutOfBoundsException`
(looking for item at place 2)
- Calling with 1 argument results `ArrayIndexOutOfBoundsException` (looking
for item at place 2)
- Calling with 2 arguments results `ArrayIndexOutOfBoundsException` (looking
for 3)
- Calling with 3 arguments results run without error message but no result
(freshly built nifi, no data)
- Calling with 3 arguments results run with a NiFi running: contrary to my
expectations, not the first but the second argument is the number of days and
not the second but the third argument is the file.
Besides of the argument handling needs to be fixed (or clarified), I would
suggest to provide some information for the user about what is going on. (Like
the number of expected arguments; no running NiFi to execute against or alike)
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1263,6 +1276,29 @@ public DecommissionTask getDecommissionTask() {
return decommissionTask;
}
+ @Override
+ public String getNodeStatusHistoryJson(final int days) {
Review comment:
- If the dependencies allow that, I would prefer to return with a
`StatusHistoryDTO` (which is part of the API) and let `BootstrapListener`
decide if it needs this in JSON format
- Regardless of the potential change above, I find it strange in this
abstraction level (NiFiServer) to have a method like this. Neither the name or
the placement seems smooth to me. (Especially the "Json" remark) - Please see
`NiFiServer` as well.
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -275,6 +282,9 @@ public static void main(String[] args) throws IOException,
InterruptedException
case "env":
runNiFi.env();
break;
+ case "status-history":
Review comment:
One more question: in case of the file already exists, the original file
is overwritten silently. I am not sure this is the right behaviour. Could we at
least inform the user that the original will be overwritten?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/spring/StatusHistoryRepositoryFactoryBean.java
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.spring;
+
+import org.apache.nifi.controller.status.history.StatusHistoryRepository;
+import org.apache.nifi.nar.ExtensionManager;
+import org.apache.nifi.nar.NarThreadContextClassLoader;
+import org.apache.nifi.util.NiFiProperties;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+public class StatusHistoryRepositoryFactoryBean implements
FactoryBean<StatusHistoryRepository>, ApplicationContextAware {
+
+ private static final String DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION =
"org.apache.nifi.controller.status.history.VolatileComponentStatusRepository";
+
+ private ApplicationContext applicationContext;
+ private NiFiProperties nifiProperties;
+ private ExtensionManager extensionManager;
+ private StatusHistoryRepository statusHistoryRepository;
+
+ @Override
+ public StatusHistoryRepository getObject() throws Exception {
+ nifiProperties = applicationContext.getBean("nifiProperties",
NiFiProperties.class);
+ extensionManager = applicationContext.getBean("extensionManager",
ExtensionManager.class);
+ final String implementationClassName =
nifiProperties.getProperty(NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION,
DEFAULT_COMPONENT_STATUS_REPO_IMPLEMENTATION);
+ if (implementationClassName == null) {
+ throw new RuntimeException("Cannot create Status History
Repository because the NiFi Properties is missing the following property: "
+ +
NiFiProperties.COMPONENT_STATUS_REPOSITORY_IMPLEMENTATION);
+ }
+
+ try {
+ statusHistoryRepository =
NarThreadContextClassLoader.createInstance(extensionManager,
implementationClassName, StatusHistoryRepository.class, nifiProperties);
+ statusHistoryRepository.start();
+ return statusHistoryRepository;
+ } catch (final Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public Class<?> getObjectType() {
+ return StatusHistoryRepository.class;
+ }
+
+ @Override
+ public boolean isSingleton() {
Review comment:
`FactoryBean` has a default implementation, returning `true`. I am not
sure this is needed.
--
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]
