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:
[email protected]
