Bill commented on a change in pull request #7366:
URL: https://github.com/apache/geode/pull/7366#discussion_r839838844
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java
##########
@@ -74,6 +77,21 @@ 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();
+ /*
+ * The verification done here is that if the locator is present
+ * then we are not re-constructing the DistributedConfigImp which is
+ * triggered when we call the getProperties()
Review comment:
Sorry to nit-pick but this comment is misleading. Calling
`getProperteis()` doesn't trigger construction of `DistributedConfigImpl`.
Suggest something like:
> We want to verify that no `DistributedConfigImpl` is constructed. But
that's hard/impossible to mock.
> We happen to know that in the case where
`SSLConfigurationFactory.getSSLConfigForComponent()`
> constructs one, `getProperties()` is called. That's why we're asserting
`getProperties()` isn't called.
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java
##########
@@ -302,47 +304,79 @@ public void handleStop() {
public static LocatorStatusResponse statusLocator(int port, InetAddress
bindAddress)
throws IOException {
return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
- new Properties());
+ new DistributionConfigImpl(new Properties()));
}
/**
* Returns the status of the locator on the given host & port. If you have
endpoint
* identification enabled the preferred method is statusForLocator(int,
String), which
* lets you specify the locator's name that the locator has stored in its
TLS certificate
*/
- public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddress)
+ public LocatorStatusResponse statusForLocator(int port, InetAddress
bindAddressArg)
throws IOException {
- return statusLocator(port, bindAddress == null ? null :
bindAddress.getCanonicalHostName(),
- getProperties());
+ final String bindAddress =
+ bindAddressArg == null ? null : bindAddressArg.getCanonicalHostName();
+ final SSLConfig sslConfig = getSSLConfigurations(getLocator());
+ return getLocatorStatusResponse(port, bindAddress, sslConfig);
+ }
+
+ private SSLConfig getSSLConfigurations(Locator locator) {
+ final SSLConfig sslConfig;
+ if (locator == null) {
+ /*
+ * Locator is null in the case when a LocatorLauncher is created
+ * using its Builder but start is never called, hence no locator exists.
+ * Instead, this LocatorLauncher is used to get the status of the
+ * locator using PID, working dir etc.
+ * This is done in StatusLocatorCommand in gfsh
+ */
+ sslConfig = SSLConfigurationFactory.getSSLConfigForComponent(
+ getProperties(),
+ SecurableCommunicationChannel.LOCATOR);
+ } else {
+ /*
+ * If a locator is already present then the DistributedConfigImpl
+ * is not constructed again but rather taken from the locator properties.
+ * This will avoid reading the property files and possible old address
+ * verification error as those may not be valid anymore.
+ */
+ sslConfig = SSLConfigurationFactory.getSSLConfigForComponent(
+ ((InternalLocator) locator).getConfig(),
+ SecurableCommunicationChannel.LOCATOR);
+ }
+ return sslConfig;
Review comment:
Would you mind moving this `private` method in the source file so it
appears after all the `public` ones?
--
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]
