NIFI-6336 Added code to catch port value when it is 0.#3932
NIFI-6336 Added code to catch port value when it is 0.#3932markap14 merged 3 commits intoapache:masterfrom
Conversation
Added unit tests validating port value format, valid port value, and catching port value of 0.
| } | ||
| int socketPort = getClusterNodeProtocolPort(); | ||
| if (socketPort == 0) { | ||
| throw new RuntimeException("Load balance port cannot be 0. Port must be inclusively in the range [1, 65535]."); |
There was a problem hiding this comment.
The exception message indicates a problem with the Load balancing port. However, this is not checking the load balancing port - it's checking the Cluster Node Protocol Port (port used to communicate between nodes in the cluster). It does appear, though, that the exact same condition exists below, in the getClusterLoadBalanceAddress method.
It is possible, though, that restricting that may cause unit test or integration test failures. Often times, a port of 0 is used for such tests, so that the test is more portable, since there's no way to know if some specific port will be open in some other environment. Do we have a reason for explicitly disallowing it?
There was a problem hiding this comment.
Using port 0 throws an exception in the NodeIdentifier.java:204. Should we allow a port of 0 (a Java-chosen random port) for all port configurations in NiFi or require administrators to configure explicit ports?
There was a problem hiding this comment.
@markap14 According to NodeIdentifier.java:204, the method checks if the port is within range 1-65535. But It seems Java will handle a port 0. So, if there isn't another reason to disallow it, are we ok to change port configurations to allow port 0?
There was a problem hiding this comment.
Ahh, you've got a good point :) We cannot allow the cluster protocol port or the web (http/https) port to be set to 0 because those needs to be consistent across restarts of the application. For site-to-site port and load balancing port, these are obtained by communicating over the cluster protocol/web protocol. So if it makes things easier to just say you can't use port of 0, then that's fine. If anyone ever actually needs to, it can be revisited then. Just didn't want to restrict it if there was no reason. "It's a non-trivial amount of work" or "It's out of scope for this Jira" are perfectly valid reasons :)
There was a problem hiding this comment.
@markap14 Great. Will correct the Exception message.
|
@markap14 I corrected the Exception message to indicate the correct port. |
|
|
||
| // Assert | ||
| // Expect NumberFormatException thrown | ||
| assertEquals(Integer.parseInt(portValue), clusterProtocolPort.intValue()); |
There was a problem hiding this comment.
This unit test is failing. NumberFormatException is not thrown. A value of 8000 should (and does) parse correctly.
|
Resolved unit test failing by providing an invalid port value to throw expected NumberFormatException. |
|
Thanks @mtien-apache ! +1 merged to master. |
3 participants
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Description of PR
Added code to catch port value when it is 0. Added unit tests to validate port values.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically
master)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.