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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
