Bill commented on a change in pull request #7366:
URL: https://github.com/apache/geode/pull/7366#discussion_r836759829
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -312,37 +314,59 @@ public static LocatorStatusResponse statusLocator(int
port, InetAddress bindAddr
*/
public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddress)
throws IOException {
- return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
- getProperties());
+ if (getLocator() != null) {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ ((InternalLocator) getLocator()).getConfig());
+ } else {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ getProperties());
+ }
Review comment:
please see my comment on the newly-introduced overload to this method
(below)…
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -312,37 +314,59 @@ public static LocatorStatusResponse statusLocator(int
port, InetAddress bindAddr
*/
public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddress)
throws IOException {
- return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
- getProperties());
+ if (getLocator() != null) {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ ((InternalLocator) getLocator()).getConfig());
+ } else {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ getProperties());
+ }
}
/**
* Returns the status of the locator on the given host & port
*/
public LocatorStatusResponse statusForLocator(int port, String hostname)
throws IOException {
- return statusLocator(port, hostname, getProperties());
+ if (getLocator() != null) {
+ return statusLocator(port, hostname, ((InternalLocator)
getLocator()).getConfig());
+ } else {
+ return statusLocator(port, hostname, getProperties());
+ }
+ }
+
+ @VisibleForTesting
+ private static LocatorStatusResponse statusLocator(
+ final int port, String bindAddress, DistributionConfigImpl
distributionConfig)
+ throws IOException {
+
+ final SSLConfig sslConfig =
SSLConfigurationFactory.getSSLConfigForComponent(
+ distributionConfig,
+ SecurableCommunicationChannel.LOCATOR);
+ return getLocatorStatusResponse(port, bindAddress, sslConfig);
}
private static LocatorStatusResponse statusLocator(
- final int port, String bindAddress,
- final Properties properties)
+ final int port, String bindAddress, Properties properties)
throws IOException {
+ final SSLConfig sslConfig =
SSLConfigurationFactory.getSSLConfigForComponent(
+ properties,
+ SecurableCommunicationChannel.LOCATOR);
+ return getLocatorStatusResponse(port, bindAddress, sslConfig);
+ }
+
+ private static LocatorStatusResponse getLocatorStatusResponse(int port,
String bindAddress,
+ SSLConfig sslConfig)
+ throws IOException {
Review comment:
This method signature change is good: taking an `SSLConfig` rather than
a `Properties` allows callers to handle the decisions about how to produce an
`SSLConfig`
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
##########
@@ -74,6 +77,16 @@ public void startReturnsOnline() {
assertThat(launcher.start().getStatus()).isEqualTo(ONLINE);
}
+ @Test
+ public void whenLocatorIsPresetThenItWillNotReadThePropertyFiles() {
+ launcher = givenLocatorLauncher();
+
+ assertThat(launcher.start().getStatus()).isEqualTo(ONLINE);
+ LocatorLauncher locatorLauncher = spy(launcher);
+ locatorLauncher.status();
+ verify(locatorLauncher, times((0))).getProperties();
Review comment:
I wish this test was more direct. What we really want to verify is that
no `DistributionConfigImpl` is constructed.
I understand that's not feasible with Assert-J. So I recommend a comment
like this:
```java
/*
What we really want to verify is that no new DistributionConfigImpl is
constructed.
But that's had/impossible to verify with Assert-J so we're approximating
that check.
See getSslConfig(), which calls getProperties() in situations where a new
DistributionConfigImpl is constructed.
*/
```
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -312,37 +314,59 @@ public static LocatorStatusResponse statusLocator(int
port, InetAddress bindAddr
*/
public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddress)
throws IOException {
- return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
- getProperties());
+ if (getLocator() != null) {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ ((InternalLocator) getLocator()).getConfig());
+ } else {
+ return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
+ getProperties());
+ }
}
/**
* Returns the status of the locator on the given host & port
*/
public LocatorStatusResponse statusForLocator(int port, String hostname)
throws IOException {
- return statusLocator(port, hostname, getProperties());
+ if (getLocator() != null) {
+ return statusLocator(port, hostname, ((InternalLocator)
getLocator()).getConfig());
+ } else {
+ return statusLocator(port, hostname, getProperties());
+ }
+ }
+
+ @VisibleForTesting
+ private static LocatorStatusResponse statusLocator(
+ final int port, String bindAddress, DistributionConfigImpl
distributionConfig)
+ throws IOException {
+
+ final SSLConfig sslConfig =
SSLConfigurationFactory.getSSLConfigForComponent(
+ distributionConfig,
+ SecurableCommunicationChannel.LOCATOR);
+ return getLocatorStatusResponse(port, bindAddress, sslConfig);
Review comment:
Instead of adding a _fourth_ `statusForLocator` overload, I recommend
adding this method (including comments):
```java
private SSLConfig getSslConfig(final InternalLocator locator) {
final SSLConfig sslConfig;
if (locator == null) {
/*
getSSLConfigForComponent will construct a new DistributionConfigImpl
and will perform validations on those configuration settings.
*/
sslConfig = SSLConfigurationFactory.getSSLConfigForComponent(
getProperties(),
SecurableCommunicationChannel.LOCATOR);
} else {
/*
In this case getSSLConfigForComponent will NOT construct a new
DistributionConfigImpl.
*/
sslConfig = SSLConfigurationFactory.getSSLConfigForComponent(
locator.getConfig(),
SecurableCommunicationChannel.LOCATOR);
}
return sslConfig;
}
```
With that method in place, the existing two non-deprecated
`statusForLocator` methods would become:
```java
public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddressArg)
throws IOException {
final String bindAddress = bindAddressArg == null ? null :
bindAddressArg.getCanonicalHostName();
final SSLConfig sslConfig = getSslConfig((InternalLocator)getLocator());
return statusLocator(port, bindAddress, sslConfig);
}
```
and
```java
public LocatorStatusResponse statusForLocator(int port, String hostname)
throws IOException {
final SSLConfig sslConfig = getSslConfig((InternalLocator)getLocator());
return statusLocator(port, hostname, sslConfig);
}
```
Doing it this way not only reduces the number of overloads (improving
readability) but also has less repeated code since we test the locator
reference for `null` in only one place. It also has the minor improvement that
we call `getLocator()` only once, instead of calling it in the `if` conditional
and again in the body of the `if`.
It also gives us a single place (the `getSslConfig` method) to document
_why_ we're doing the `null`-check.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
