Skip to content

NIFI-5653 Added default NiFi and Embedded Zookeeper port tables to Ad…#3089

Closed
andrewmlim wants to merge 2 commits intoapache:masterfrom
andrewmlim:NIFI-5653
Closed

NIFI-5653 Added default NiFi and Embedded Zookeeper port tables to Ad…#3089
andrewmlim wants to merge 2 commits intoapache:masterfrom
andrewmlim:NIFI-5653

Conversation

@andrewmlim
Copy link
Contributor

…min Guide

Also added a new referenced tag/anchor for TLS Generation Toolkit section and removed an unnecessary NOTE.

Copy link
Contributor

@jtstorck jtstorck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing...

|==================================================================================================================================================
| Function | Property | Default Value
|Zookeeper Client Port | `clientPort` | `2181`
|Zookeeper Server Quorum and Leader Election Ports | `server.1` | `localhost:2888:3888`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In zookeeper.properties, the default values are commented:

# server.1=nifi-node1-hostname:2888:3888
# server.2=nifi-node2-hostname:2888:3888
# server.3=nifi-node3-hostname:2888:3888

Technically this means that the default values for server.1 to server.N are empty. The note below references that examples are commented out, which is good, but I think the port list should indicate that the server.N properties are empty by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. We could change the table and following note to this:

| Function | Property | Default Value
|Zookeeper Client Port | clientPort | 2181
|Zookeeper Server Quorum and Leader Election Ports | server.1 | blank

NOTE: Commented examples for the Zookeeper server ports are included in the zookeeper.properties file in the form server.N=nifi-nodeN-hostname:2888:3888.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, with blank being changed to none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will make this change also.

|Remote Input Socket Port* | `nifi.remote.input.socket.port` | `10443`
|Cluster Node Protocol Port* | `nifi.cluster.node.protocol.port` | `11443`
|Cluster Node Load Balancing Port | `nifi.cluster.node.load.balance.port` | `6342`
|Web HTTP Forwarding Port | `nifi.web.http.port.forwarding` | blank
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we represent no default value here other than "blank"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that also, but figured "blank" would be OK since we reference this throughout the Admin Guide (e.g. "The default value is blank").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in other comment threads, changing blank to none.

|Web HTTP Forwarding Port | `nifi.web.http.port.forwarding` | blank
|==================================================================================================================================================

NOTE: The ports marked with an asterisk (*) have property values that are blank by default in _nifi.properties_. The values shown in the table are the default values for these ports when <<tls_generation_toolkit>> is used to generate _nifi.properties_ for a secured NiFi instance. The default Certificate Authority Port used by TLS Toolkit is `8443`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NOTE: The ports marked with an asterisk (*) have property values that are blank by default in _nifi.properties_. The values shown in the table are the default values for these ports when <<tls_generation_toolkit>> is used to generate _nifi.properties_ for a secured NiFi instance. The default Certificate Authority Port used by TLS Toolkit is `8443`.
NOTE: The ports marked with an asterisk (*) have property values that are empty by default in _nifi.properties_. The values shown in the table are the default values for these ports when <<tls_generation_toolkit>> is used to generate _nifi.properties_ for a secured NiFi instance. The default Certificate Authority Port used by TLS Toolkit is `8443`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to comment above about "blank", but happy to make this change since it reads better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty is probably not correct here. It could imply an empty string, which would not be the case for a property set like nifi.web.http.port.forwarding= as it is by default in nifi.properties. blank is probably the better option to use in the descriptions, but maybe null or none in the port lists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will make none in the tables.

@jtstorck
Copy link
Contributor

+1 LGTM. Merging! Thanks for this documentation, @andrewmlim!

@asfgit asfgit closed this in 5ec8529 Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants