You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/02/16 18:40:29 UTC
[GitHub] [geode] kirklund commented on a change in pull request #5970: GEODE-8877: Configurable membership bind address
kirklund commented on a change in pull request #5970:
URL: https://github.com/apache/geode/pull/5970#discussion_r577013420
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/direct/DirectChannel.java
##########
@@ -142,7 +143,23 @@ public DirectChannel(Membership<InternalDistributedMember> mgr,
props.setProperty("membership_port_range_start", "" + range[0]);
props.setProperty("membership_port_range_end", "" + range[1]);
- this.conduit = new TCPConduit(mgr, port, address, isBindAddress, this, bufferPool, props);
+ InetAddress conduitAddress = address;
+ if (!dc.getMembershipBindAddress().isEmpty()) {
+ try {
+ if (dc.getMembershipBindAddress().equals("*")) {
+ conduitAddress = (new InetSocketAddress(0)).getAddress();
+ } else {
+ conduitAddress = InetAddress.getByName(dc.getMembershipBindAddress());
+ }
+ } catch (UnknownHostException e) {
+ logger.error("Exception when configuring " + dc.getMembershipBindAddress()
Review comment:
It's better if all of our log statements use Log4j2 parameterization for consistency:
```
logger.error("Exception when using {} as bind address in membership, default address will be used: ", dc.getMembershipBindAddress(), e.getMessage());
```
If the log level is INFO or greater (ERROR, etc) then please make sure the log statement makes sense to users and not just devs (you and me). For example, you might want to swap out the word `DirectChannel` for just `membership`. If `DirectChannel` was a User API (not internal), then it might make more sense to use that class name in the log statement.
Are you sure that `getMessage()` for `UnknownHostException` returns the `default address`? Also, please note that the exact string for the same java exception message can vary from JVM to JVM or even the same JVM on different OS'es.
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -104,7 +104,7 @@ public void before() {
@Test
public void testGetAttributeNames() {
String[] attNames = AbstractDistributionConfig._getAttNames();
- assertThat(attNames.length).isEqualTo(169);
+ assertThat(attNames.length).isEqualTo(170);
Review comment:
AssertJ usually has several different ways to assert something and each one has a different failure message. I usually use whatever produces the most informative error message.
Failure of `assertThat(attNames.length).isEqualTo(170);` produces:
```
org.junit.ComparisonFailure:
Expected :170
Actual :169
...
at org.apache.geode.distributed.internal.DistributionConfigJUnitTest.foo(DistributionConfigJUnitTest.java:502)
...
```
Failure of `assertThat(attNames).hasSize(170);` produces:
```
java.lang.AssertionError:
Expected size:<170> but was:<169> in:
<["ack-severe-alert-threshold",
"ack-wait-threshold",
...<snip>
"user-command-packages",
"validate-serializable-objects"]>
at org.apache.geode.distributed.internal.DistributionConfigJUnitTest.foo(DistributionConfigJUnitTest.java:502)
...
```
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
##########
@@ -120,25 +121,32 @@ public ResultModel startLocator(
help = CliStrings.START_LOCATOR__HTTP_SERVICE_BIND_ADDRESS__HELP) final String httpServiceBindAddress,
@CliOption(key = CliStrings.START_LOCATOR__REDIRECT_OUTPUT, unspecifiedDefaultValue = "false",
specifiedDefaultValue = "true",
- help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput)
+ help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput,
+ @CliOption(key = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS,
+ help = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS__HELP) final String membershipBindAddress)
throws Exception {
if (StringUtils.isBlank(memberName)) {
// when the user doesn't give us a name, we make one up!
memberName = StartMemberUtils.getNameGenerator().generate('-');
}
- workingDirectory = StartMemberUtils.resolveWorkingDir(
- workingDirectory == null ? null : new File(workingDirectory), new File(memberName));
+ workingDirectory = resolveWorkingDirectory(workingDirectory, memberName);
return doStartLocator(memberName, bindAddress, classpath, force, group, hostnameForClients,
jmxManagerHostnameForClients, includeSystemClasspath, locators, logLevel, mcastBindAddress,
mcastPort, port, workingDirectory, gemfirePropertiesFile, gemfireSecurityPropertiesFile,
initialHeap, maxHeap, jvmArgsOpts, connect, enableSharedConfiguration,
loadSharedConfigurationFromDirectory, clusterConfigDir, httpServicePort,
- httpServiceBindAddress, redirectOutput);
+ httpServiceBindAddress, redirectOutput, membershipBindAddress);
}
+ @VisibleForTesting
+ protected static String resolveWorkingDirectory(String workDirValue, String memberName) {
Review comment:
`StartMemberUtils` is internal, so feel free to change method signatures in there or rename `resolveWorkingDir` to `resolveWorkingDirectory`. It's generally always best to spell out words rather than use abbreviations.
While you're in this code, I would also change `StartMemberUtils` and its constant `GEODE_HOME` to package-private. The only uses of this class are from within the same package.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -1738,6 +1769,37 @@ public Builder setPort(final Integer port) {
return this;
}
+ /**
+ * Sets the IP address to be used for membership-related traffic binding.
+ *
+ * @param membershipBindAddress a String containing the IP address to be used for
+ * membership-related traffic binding.
+ * @return this Builder instance.
+ * @see #getMembershipBindAddress()
+ */
+ public Builder setMembershipBindAddress(final String membershipBindAddress) {
+ this.membershipBindAddress = membershipBindAddress;
+ return this;
+ }
+
+ /**
+ * Gets the IP address to be used for membership-related traffic binding.
+ *
+ * @return a String containing the IP address to be used for membership-related traffic binding.
+ */
+ public String getMembershipBindAddress() {
+ return this.membershipBindAddress;
+ }
+
+ /**
+ * Determines whether the membership-bind-address property is defined or not.
+ *
+ * @return a boolean value indicating if the membership-bind-address property is defined or not.
+ */
+ public boolean membershipBindAddressSpecified() {
Review comment:
Since this is a User API, I recommend staying consistent with the other Builder methods, such as `isBindAddressSpecified()`, which would make this one `isMembershipBindAddressSpecified()`.
Any class or inner-class that is in a package without the word `internal` is a User API (unless the class or methods are package-private).
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
##########
@@ -120,25 +121,32 @@ public ResultModel startLocator(
help = CliStrings.START_LOCATOR__HTTP_SERVICE_BIND_ADDRESS__HELP) final String httpServiceBindAddress,
@CliOption(key = CliStrings.START_LOCATOR__REDIRECT_OUTPUT, unspecifiedDefaultValue = "false",
specifiedDefaultValue = "true",
- help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput)
+ help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput,
+ @CliOption(key = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS,
+ help = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS__HELP) final String membershipBindAddress)
throws Exception {
if (StringUtils.isBlank(memberName)) {
// when the user doesn't give us a name, we make one up!
memberName = StartMemberUtils.getNameGenerator().generate('-');
}
- workingDirectory = StartMemberUtils.resolveWorkingDir(
- workingDirectory == null ? null : new File(workingDirectory), new File(memberName));
+ workingDirectory = resolveWorkingDirectory(workingDirectory, memberName);
return doStartLocator(memberName, bindAddress, classpath, force, group, hostnameForClients,
jmxManagerHostnameForClients, includeSystemClasspath, locators, logLevel, mcastBindAddress,
mcastPort, port, workingDirectory, gemfirePropertiesFile, gemfireSecurityPropertiesFile,
initialHeap, maxHeap, jvmArgsOpts, connect, enableSharedConfiguration,
loadSharedConfigurationFromDirectory, clusterConfigDir, httpServicePort,
- httpServiceBindAddress, redirectOutput);
+ httpServiceBindAddress, redirectOutput, membershipBindAddress);
}
+ @VisibleForTesting
+ protected static String resolveWorkingDirectory(String workDirValue, String memberName) {
Review comment:
Do you really need the both `StartLocatorCommand.resolveWorkingDirectory(String, String)` and `StartMemberUtils.resolveWorkingDir`? I think you should either remove the new method or move it down to `StartMemberUtils`.
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java
##########
@@ -953,8 +955,23 @@ public void init(Services<ID> s) throws MembershipConfigurationException {
public void started() throws MemberStartupException {
setLocalAddress(services.getMessenger().getMemberID());
try {
- serverSocket = createServerSocket(localAddress.getInetAddress(),
- services.getConfig().getMembershipPortRange());
+ InetAddress address = localAddress.getInetAddress();
+ if (services.getConfig().getMembershipBindAddress() != null
+ && !services.getConfig().getMembershipBindAddress().isEmpty()) {
+ try {
+ if (services.getConfig().getMembershipBindAddress().equals("*")) {
+ address = (new InetSocketAddress(0)).getAddress();
+ } else {
+ address = InetAddress.getByName(services.getConfig().getMembershipBindAddress());
+ }
+ } catch (UnknownHostException e) {
+ logger
Review comment:
Same logger syntax issues as mentioned previously.
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java
##########
@@ -140,7 +140,7 @@ public void testGetAttributeNames() {
// are.
assertEquals(36, boolList.size());
assertEquals(35, intList.size());
- assertEquals(88, stringList.size());
+ assertEquals(89, stringList.size());
Review comment:
I generally update every test I touch to use AssertJ instead of JUnit Assertions. There's a great IntelliJ plugin that will do the bulk of the work for you called **Assertions2AssertJ**. For example, using this plugin adds a new menu item to "**Refactor -> Convert Assertions to AssertJ**".
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartLocatorCommand.java
##########
@@ -120,25 +121,32 @@ public ResultModel startLocator(
help = CliStrings.START_LOCATOR__HTTP_SERVICE_BIND_ADDRESS__HELP) final String httpServiceBindAddress,
@CliOption(key = CliStrings.START_LOCATOR__REDIRECT_OUTPUT, unspecifiedDefaultValue = "false",
specifiedDefaultValue = "true",
- help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput)
+ help = CliStrings.START_LOCATOR__REDIRECT_OUTPUT__HELP) final Boolean redirectOutput,
+ @CliOption(key = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS,
+ help = CliStrings.START_LOCATOR__MEMBERSHIP_BIND_ADDRESS__HELP) final String membershipBindAddress)
throws Exception {
if (StringUtils.isBlank(memberName)) {
// when the user doesn't give us a name, we make one up!
memberName = StartMemberUtils.getNameGenerator().generate('-');
}
- workingDirectory = StartMemberUtils.resolveWorkingDir(
- workingDirectory == null ? null : new File(workingDirectory), new File(memberName));
+ workingDirectory = resolveWorkingDirectory(workingDirectory, memberName);
return doStartLocator(memberName, bindAddress, classpath, force, group, hostnameForClients,
jmxManagerHostnameForClients, includeSystemClasspath, locators, logLevel, mcastBindAddress,
mcastPort, port, workingDirectory, gemfirePropertiesFile, gemfireSecurityPropertiesFile,
initialHeap, maxHeap, jvmArgsOpts, connect, enableSharedConfiguration,
loadSharedConfigurationFromDirectory, clusterConfigDir, httpServicePort,
- httpServiceBindAddress, redirectOutput);
+ httpServiceBindAddress, redirectOutput, membershipBindAddress);
}
+ @VisibleForTesting
+ protected static String resolveWorkingDirectory(String workDirValue, String memberName) {
+ return StartMemberUtils.resolveWorkingDir(
+ workDirValue == null ? null : new File(workDirValue), new File(memberName));
Review comment:
I might override `resolveWorkingDir` with multiple signatures so you can get rid of the use of nulls and ternary operators that just check null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org