Skip to content
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

PAYARA-3457 Added Config Attribute to Enable/Disable Port Auto Increment #3728

Merged
merged 4 commits into from Feb 19, 2019

Conversation

Projects
None yet
4 participants
@jbee
Copy link
Contributor

jbee commented Feb 14, 2019

Added setting to HazelcastRuntimeConfiguration as well as the admingui. Default is true.

@jbee jbee added this to the 5.191 milestone Feb 14, 2019

@jbee jbee self-assigned this Feb 14, 2019

@jbee jbee requested review from Pandrex247 , smillidge and MeroRai Feb 14, 2019

@@ -454,6 +451,7 @@ private void buildNetworkConfiguration(Config config) throws NumberFormatExcepti
port = Integer.valueOf(configuration.getDasPort());
}
config.getNetworkConfig().setPort(port);
config.getNetworkConfig().setPortAutoIncrement("true".equalsIgnoreCase(configuration.getPortAutoIncrement()));

This comment has been minimized.

@jbee

jbee Feb 14, 2019

Author Contributor

Note: I am not a 100% sure we always should forward the setting from the config but on the other hand I can't see a reason to not do it. It would be interesting to know why we previously only set port auto increment to true for multicast.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 14, 2019

jenkins test please

jbee added some commits Feb 14, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 14, 2019

jenkins test please

1 similar comment
@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 14, 2019

jenkins test please

@Pandrex247
Copy link
Member

Pandrex247 left a comment

Naming quibbles

@@ -201,6 +201,9 @@
@Param(name = "kubernetesServiceName", optional = true, alias = "kubernetesservicename")
private String kubernetesServiceName;

@Param(name = "enableAutoPortSelection", optional = true, defaultValue = "true")

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

I wouldn't have a default value of "true" here.
It means that you always have to specify it if you want to keep it disabled.

So if I did this:
asadmin set-hazelcast-configuration --enableAutoPortSelection false --lite true
followed by this:
asadmin set-hazelcast-configuration --memberName wibbles
the end result would be that the auto-port selection would still be enabled.

This comment has been minimized.

@jbee

jbee Feb 18, 2019

Author Contributor

Agreed. Had not thought about this.

@@ -201,6 +201,9 @@
@Param(name = "kubernetesServiceName", optional = true, alias = "kubernetesservicename")
private String kubernetesServiceName;

@Param(name = "enableAutoPortSelection", optional = true, defaultValue = "true")
private boolean enableAutoPortSelection;

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

This could also be renamed to just be autoIncrementPort to make it sound less robotic.
Also auto-port selection sounds scary, like it's just going to pick one at random :P

This comment has been minimized.

@jbee

jbee Feb 18, 2019

Author Contributor

I like that better as well, the enableAutoPortSelection came from the ticket but I assume it can treat it as a suggestion and use autoIncrementPort instead.

@@ -109,6 +109,7 @@ public void execute(AdminCommandContext context) {
ColumnFormatter columnFormatter = new ColumnFormatter(headers);
columnFormatter.addRow(new Object[]{"Configuration File",runtimeConfiguration.getHazelcastConfigurationFile(),"Domain"});
columnFormatter.addRow(new Object[]{"Interfaces",runtimeConfiguration.getInterface(),"Domain"});
columnFormatter.addRow(new Object[]{"Port Auto Increment", runtimeConfiguration.getPortAutoIncrement(), "Domain"});

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

"Auto Increment Port" reads a bit better.

This comment has been minimized.

@jbee

jbee Feb 18, 2019

Author Contributor

Its called "Port Auto Increment" in hazelcast so I would think avoiding confusion caused by using a very similar name is more important than something that reads better? WDYT?

@@ -56,6 +56,10 @@
String getHazelcastConfigurationFile();
public void setHazelcastConfigurationFile(String value);

@Attribute(defaultValue = "true", dataType = Boolean.class)
String getPortAutoIncrement();

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

See other comments.

Show resolved Hide resolved ...dmingui/payara-console-extras/src/main/resources/hazelcast/hazelcast.jsf Outdated
@@ -94,6 +94,8 @@ hazelcast.members.members =Data Grid Instances
hazelcast.members.membersHelp=The Data Grid Server Instances
hazelcast.configuration.dynamic=Restart Data Grid
hazelcast.configuration.dynamicHelp=Applies the data grid configuration change immediately, without needing a restart of the Server.<br/>WARNING this can cause data grid wide data loss if a large number of instances restart the data grid immediately.
hazelcast.configuration.enableAutoPortSelection=Port Auto Increment

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

See other naming comments

@@ -94,6 +94,8 @@ hazelcast.members.members =Data Grid Instances
hazelcast.members.membersHelp=The Data Grid Server Instances
hazelcast.configuration.dynamic=Restart Data Grid
hazelcast.configuration.dynamicHelp=Applies the data grid configuration change immediately, without needing a restart of the Server.<br/>WARNING this can cause data grid wide data loss if a large number of instances restart the data grid immediately.
hazelcast.configuration.enableAutoPortSelection=Port Auto Increment
hazelcast.configuration.enableAutoPortSelectionHelp=When enabled the data grid automatically increments the port to next available port should the start port be occupied already.

This comment has been minimized.

@Pandrex247

Pandrex247 Feb 18, 2019

Member

I'd make the following changes to this:
"When enabled the data grid automatically increments the port to *the* next available *port* should the start port be occupied *already*."

I think it reads a bit better like that.

@@ -54,7 +54,7 @@
<!beforeCreate
gf.restRequest(endpoint="#{sessionScope.REST_URL}/get-hazelcast-configuration" method="GET" result="#{requestScope.resp}");
setPageSessionAttribute(key="valueMap", value="#{requestScope.resp.data.extraProperties.getHazelcastConfiguration}");
setPageSessionAttribute(key="convertToFalseList", value={"enabled","dynamic"});
setPageSessionAttribute(key="convertToFalseList", value={"enabled","dynamic","autoIncrementPort","hostAwarePartitioning"});

This comment has been minimized.

@jbee

jbee Feb 18, 2019

Author Contributor

Note the addition of "hostAwarePartitioning" to the list which fixed a defect where the unchecked checkbox would not change the state of the configuration.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 18, 2019

Renamed to autoIncrementPort done.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Feb 18, 2019

jenkins test please

@Pandrex247 Pandrex247 merged commit a00a365 into payara:master Feb 19, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.