[GitHub] [geode] DonalEvans commented on a change in pull request #5948: GEODE-8872: added client impacts

[GitBox <gi...@apache.org>]

DonalEvans commented on a change in pull request #5948:
URL: https://github.com/apache/geode/pull/5948#discussion_r585940678



##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
##########
@@ -207,6 +207,13 @@ default boolean getThreadLocalConnections() {
    */
   boolean getMultiuserAuthentication();
 
+  /**
+   * Returns true ir request locator internal address is enabled on this pool.
+   *
+   * @see PoolFactory#setRequestLocatorInternalAddress(boolean)
+   * @since Geode 1.14
+   */
+  boolean getRequestLocatorInternalAddress();

Review comment:
       This method name is a little confusing, as it might be mistaken for a getter for the locator internal address itself. It might be better as "isRequestLocatorInternalAddressEnabled", and the corresponding setter as "setRequestLocatorInternalAddressEnabled".

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -257,6 +257,9 @@ public Set adviseProfileRemove() {
 
     private String host;
 
+    /** valid only for ControllerProfile */
+    private String internalHost;

Review comment:
       It doesn't seem correct to have a field in a parent class that only belongs to one of its child classes. Can this be moved into ControllerProfile?

##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/PoolFactory.java
##########
@@ -220,6 +220,14 @@
    * @since GemFire 6.5
    */
   boolean DEFAULT_MULTIUSER_AUTHENTICATION = false;
+  /**
+   * The default value for whether to request locator internal address.

Review comment:
       Slightly more detail would be good here, explaining when exactly the locator internal address would be requested if this was set to true.

##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/Pool.java
##########
@@ -207,6 +207,13 @@ default boolean getThreadLocalConnections() {
    */
   boolean getMultiuserAuthentication();
 
+  /**
+   * Returns true ir request locator internal address is enabled on this pool.

Review comment:
       Typo here, should be "Returns true if..."

##########
File path: geode-dunit/src/main/java/org/apache/geode/cache/client/internal/LocatorTestBase.java
##########
@@ -141,6 +141,34 @@ protected int startLocator(final String hostName, final String otherLocators) th
     return locator.getPort();
   }
 
+  protected int startLocator(final String hostName, final String otherLocators,

Review comment:
       This method is never used.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -432,6 +447,17 @@ public boolean equals(Object obj) {
         if (this.gp.getPort() == other.gp.getPort()) {
           final String thisHost = this.gp.getHost();
           final String otherHost = other.gp.getHost();
+          final String thisInternalHost = this.gp.getInternalHost();

Review comment:
       If internalHost is being included in `equals()` calculations, it should probably also be included in hashCode()`.




----------------------------------------------------------------
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