[GitHub] [geode] Bill commented on a change in pull request #5680: GEODE-8623: Retry getting local host if it fails.

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

Bill commented on a change in pull request #5680:
URL: https://github.com/apache/geode/pull/5680#discussion_r521740981



##########
File path: geode-common/src/main/java/org/apache/geode/internal/Retry.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal;
+
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Utility class for retrying operations.
+ */
+public class Retry {
+
+  interface Clock {
+    long nanoTime();
+  }
+
+  private static class SteadyClock implements Clock {
+    @Override
+    public long nanoTime() {
+      return System.nanoTime();
+    }
+  }
+
+  private static final SteadyClock steadyClock = new SteadyClock();
+
+  /**
+   * Try the supplier function until the predicate is true or timeout occurs.
+   *
+   * @param supplier to execute until predicate is true or times out
+   * @param predicate to test for retry
+   * @param timeout to retry for
+   * @param timeUnit to retry for
+   * @param <T> type of return value
+   * @return value from supplier after it passes predicate or times out.
+   */
+  public static <T> T tryFor(final Supplier<T> supplier, final Predicate<T> predicate,
+      final long timeout, final TimeUnit timeUnit) throws TimeoutException {
+    return tryFor(supplier, predicate, timeout, timeUnit, steadyClock);
+  }
+
+  static <T> T tryFor(final Supplier<T> supplier, final Predicate<T> predicate,
+      final long timeout, final TimeUnit timeUnit, final Clock clock)
+      throws TimeoutException {
+    long until = clock.nanoTime() + NANOSECONDS.convert(timeout, timeUnit);
+
+    T value;
+    do {
+      value = supplier.get();
+      if (predicate.test(value)) {
+        return value;
+      }
+    } while (clock.nanoTime() < until);

Review comment:
       This is a hot loop unless `supplier` or `predicate` sleeps. On the one hand that's cool because it leave sleeping outside this class. On the other hand, um, it might be nice to have this structured to not have a hot loop by default.
   
   Similar to the way the `Clock` implementation is injected, a sleeper could be injected, if you wanted to do that, maybe you could:
   
   ```
   @FunctionalInterface
   public interface Sleeper {
    void sleep() throw InterruptedException;
   }
   ```
   and after `predicate` evaluates to `false`:
   
   ```
   sleeper.sleep(...)
   ```
   Where the sleep time comes from parameters the way `timeout` et al. do now.
   
   Note that some sophistication could be added to clamp the sleep time so as to try and not overflow `until` by too much. Then again, that might turn into a fools errand.

##########
File path: geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -54,70 +59,80 @@
   private static final boolean useLinkLocalAddresses =
       Boolean.getBoolean(USE_LINK_LOCAL_ADDRESSES_PROPERTY);
 
-  /**
-   * we cache localHost to avoid bug #40619, access-violation in native code
-   */
-  private static final InetAddress localHost;
-
   /**
    * all classes should use this variable to determine whether to use IPv4 or IPv6 addresses
    */
   @MakeNotStatic
   private static boolean useIPv6Addresses = !Boolean.getBoolean("java.net.preferIPv4Stack")
       && Boolean.getBoolean("java.net.preferIPv6Addresses");
 
-  static {
-    InetAddress inetAddress = null;
+  /**
+   * Resolves local host. Will retry if resolution fails.
+   *
+   * @return local host if resolved otherwise null.
+   */
+  private static InetAddress resolveLocalHost() {
     try {
-      inetAddress = InetAddress.getByAddress(InetAddress.getLocalHost().getAddress());
-      if (inetAddress.isLoopbackAddress()) {
-        InetAddress ipv4Fallback = null;
-        InetAddress ipv6Fallback = null;
-        // try to find a non-loopback address
-        Set<InetAddress> myInterfaces = getMyAddresses();
-        boolean preferIPv6 = useIPv6Addresses;
-        String lhName = null;
-        for (InetAddress addr : myInterfaces) {
-          if (addr.isLoopbackAddress() || addr.isAnyLocalAddress() || lhName != null) {
-            break;
-          }
-          boolean ipv6 = addr instanceof Inet6Address;
-          boolean ipv4 = addr instanceof Inet4Address;
-          if ((preferIPv6 && ipv6) || (!preferIPv6 && ipv4)) {
-            String addrName = reverseDNS(addr);
-            if (inetAddress.isLoopbackAddress()) {
-              inetAddress = addr;
-              lhName = addrName;
-            } else if (addrName != null) {
-              inetAddress = addr;
-              lhName = addrName;
+      return tryFor(() -> {

Review comment:
       This looks like a hot loop. See comments above on `tryFor()`

##########
File path: geode-common/src/main/java/org/apache/geode/internal/Retry.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal;
+
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Utility class for retrying operations.
+ */
+public class Retry {

Review comment:
       So many cool directions this could take, for use-cases beyond the current one. Thinking of back-off policies, for instance.

##########
File path: geode-common/src/main/java/org/apache/geode/internal/Retry.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal;
+
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Utility class for retrying operations.
+ */
+public class Retry {
+
+  interface Clock {
+    long nanoTime();
+  }
+
+  private static class SteadyClock implements Clock {
+    @Override
+    public long nanoTime() {
+      return System.nanoTime();
+    }
+  }
+
+  private static final SteadyClock steadyClock = new SteadyClock();
+
+  /**
+   * Try the supplier function until the predicate is true or timeout occurs.
+   *
+   * @param supplier to execute until predicate is true or times out
+   * @param predicate to test for retry
+   * @param timeout to retry for
+   * @param timeUnit to retry for
+   * @param <T> type of return value
+   * @return value from supplier after it passes predicate or times out.
+   */
+  public static <T> T tryFor(final Supplier<T> supplier, final Predicate<T> predicate,

Review comment:
       Looking at the call sites for these methods, I wonder if those would be easier to understand if the non-lambda args came first? Not adamant about that but sometimes it makes things easier to read.
   
   I usually put functions last in the ruby (block) style when a method takes a single one. Since this method takes two functions it may be beyond help though.

##########
File path: geode-common/src/main/java/org/apache/geode/internal/Retry.java
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal;
+
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+
+/**
+ * Utility class for retrying operations.
+ */
+public class Retry {
+
+  interface Clock {
+    long nanoTime();
+  }
+
+  private static class SteadyClock implements Clock {
+    @Override
+    public long nanoTime() {
+      return System.nanoTime();
+    }
+  }
+
+  private static final SteadyClock steadyClock = new SteadyClock();
+
+  /**
+   * Try the supplier function until the predicate is true or timeout occurs.
+   *
+   * @param supplier to execute until predicate is true or times out
+   * @param predicate to test for retry
+   * @param timeout to retry for
+   * @param timeUnit to retry for
+   * @param <T> type of return value
+   * @return value from supplier after it passes predicate or times out.
+   */
+  public static <T> T tryFor(final Supplier<T> supplier, final Predicate<T> predicate,
+      final long timeout, final TimeUnit timeUnit) throws TimeoutException {
+    return tryFor(supplier, predicate, timeout, timeUnit, steadyClock);
+  }
+
+  static <T> T tryFor(final Supplier<T> supplier, final Predicate<T> predicate,
+      final long timeout, final TimeUnit timeUnit, final Clock clock)
+      throws TimeoutException {
+    long until = clock.nanoTime() + NANOSECONDS.convert(timeout, timeUnit);
+
+    T value;
+    do {
+      value = supplier.get();
+      if (predicate.test(value)) {
+        return value;
+      }
+    } while (clock.nanoTime() < until);

Review comment:
       after writing this I saw the comments about the hot loop by others below but decided to leave it here anyway because I'm obsessed with this kind of stuff.




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