-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-13045 Provide an option to specify the bootstrap listen port #8648
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement @briansolo1985. The nifi-registry
module is commented out in the root Maven configuration, so that needs to be reverted.
52a9582
to
f4cac92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @briansolo1985.
Most of the changes look good. I noted one recommend to throw an exception instead of logging a warning for an unparsable port number, since that would be contrary to the expected behavior configured in properties.
minifi/minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/command/StartRunner.java
Outdated
Show resolved
Hide resolved
minifi/minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/command/StartRunner.java
Outdated
Show resolved
Hide resolved
minifi/minifi-bootstrap/src/main/java/org/apache/nifi/minifi/bootstrap/command/StartRunner.java
Outdated
Show resolved
Hide resolved
f4cac92
to
ba69393
Compare
Incorporated all the review comments, please check the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @briansolo1985. The latest version looks good, I just noted one minor typo, and opportunity to capitalize Bootstrap, then this looks ready to go.
} | ||
}) | ||
.orElse(0); | ||
CMD_LOGGER.info("Using boostrap listen port {}", parsedBootstrapListenPort); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor spelling correction, wording adjustment recommendation:
CMD_LOGGER.info("Using boostrap listen port {}", parsedBootstrapListenPort); | |
CMD_LOGGER.info("Bootstrap listen port {}", parsedBootstrapListenPort); |
try { | ||
return Integer.parseInt(port); | ||
} catch (NumberFormatException e) { | ||
throw new IllegalArgumentException("Unable to parse bootstrap listen port, please provide a valid port for " + NIFI_BOOTSTRAP_LISTEN_PORT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor adjustment, recommend capitalizing Bootstrap:
throw new IllegalArgumentException("Unable to parse bootstrap listen port, please provide a valid port for " + NIFI_BOOTSTRAP_LISTEN_PORT); | |
throw new IllegalArgumentException("Unable to parse Bootstrap listen port, please provide a valid port for " + NIFI_BOOTSTRAP_LISTEN_PORT); |
ba69393
to
aa15670
Compare
Updated the PR, please check and merge if good to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @briansolo1985! The latest version looks good! +1 merging
This closes apache#8648 Signed-off-by: David Handermann <exceptionfactory@apache.org>
This closes apache#8648 Signed-off-by: David Handermann <exceptionfactory@apache.org>
Summary
NIFI-13045
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation