[
https://issues.apache.org/jira/browse/NIFI-4147?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16088715#comment-16088715
]
ASF GitHub Bot commented on NIFI-4147:
--------------------------------------
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1972#discussion_r127591766
--- Diff:
nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java ---
@@ -135,8 +135,8 @@
private volatile Set<Future<?>> loggingFutures = new HashSet<>(2);
private final NotificationServiceManager serviceManager;
- public RunNiFi(final File bootstrapConfigFile, final boolean verbose)
throws IOException {
--- End diff --
I do not recommend changing the signature of a public constructor.
Although there is no other use of the RunNiFi constructor in the NiFi project,
someone might be using it in their own code, elsewhere. Is this essential to
your testability goal? It does look like we're not sure about if/how to use
`verbose`, but that might be a different ticket. If it is essential, how about
a second constructor?
With respect to the `File bootstrapConfigFile` argument, I understand there
is a difference because you make `getBootstrapConfigFile()` an instance method,
so RunNiFi will not separately call `getBootstrapConfigFile()` first. What if
we allowed that as a nullable argument, where it is accepted if given, and
looked it up if `null` is passed in? Again, I think this would preserve
compatibility with possible existing callers.
> Class org.apache.nifi.bootstrap.RunNiFi is not designed with extensibility in
> mind
> ----------------------------------------------------------------------------------
>
> Key: NIFI-4147
> URL: https://issues.apache.org/jira/browse/NIFI-4147
> Project: Apache NiFi
> Issue Type: Improvement
> Components: Core Framework
> Reporter: Peter Horvath
>
> Class org.apache.nifi.bootstrap.RunNiFi was not designed with extensibility
> in mind. This class is used to bootstrap the runtime; the current
> implementation does not support extending the class with different behaviour,
> that would be essential to bootstrap the engine within an integration test.
> This class should be changed so that it can be easily subclassed, allowing it
> to be later extended within an integration test runner:
> * The verbose parameter within the RunNiFi constructor is not used: eliminate
> it
> * getBootstrapConfFile() is static, which means it cannot easily be
> overridden in a test stub: replace with an instance method, which can be
> overridden
> * Methods getPidFile, getStatusFile, getLockFile, getStatusFile are package
> protected, preventing these methods to be stubbed for an integration test:
> make them protected so that they can be overridden from a subclass (allowing
> an integration test runner to override default behaviour in a test run)
> *Please note these changes are baby-steps towards the implementation of a
> NiFi integration test harness.*
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
