[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2825: Cache SSLContext

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

ctubbsii commented on code in PR #2825:
URL: https://github.com/apache/accumulo/pull/2825#discussion_r929234882


##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -169,7 +176,8 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
     this.serverConf = serverConf;
     timeoutSupplier = memoizeWithExpiration(
         () -> getConfiguration().getTimeInMillis(Property.GENERAL_RPC_TIMEOUT));
-    sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()));
+    sslSupplier = memoizeWithExpiration(() -> SslConnectionParams.forClient(getConfiguration()), 1,
+        TimeUnit.DAYS);

Review Comment:
   I don't think it can. I think this was just for lazy initialization, and nothing more.



##########
core/src/main/java/org/apache/accumulo/core/rpc/SslConnectionParams.java:
##########
@@ -264,7 +264,7 @@ public int hashCode() {
     }
     hash = 31 * hash + clientProtocol.hashCode();
     hash = 31 * hash + Arrays.hashCode(serverProtocols);
-    return super.hashCode();
+    return hash;

Review Comment:
   I wonder if the original intent was something like `super.hashCode() + hash`, but they left it unfinished?



##########
core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java:
##########
@@ -53,12 +54,21 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.Scheduler;
+
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /**
  * Factory methods for creating Thrift client objects
  */
 public class ThriftUtil {
+
+  private static final Cache<SslConnectionParams,SSLContext> SSL_CONTEXT_CACHE =
+      Caffeine.newBuilder().scheduler(Scheduler.systemScheduler())
+          .expireAfterAccess(Duration.ofMinutes(10)).initialCapacity(1).maximumSize(8).build();

Review Comment:
   The transport is already cached. I would expect a new SSLContext to be created for each new transport... we don't know if the underlying JCE library or some other state has changed that would result in a differently-constructed context for the same SSL params. If this is expensive, then perhaps we need to focus on caching the transport longer, rather than caching at both the SSLContext and the transport?



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -153,6 +154,12 @@ private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s) {
     return () -> Suppliers.memoizeWithExpiration(s::get, 100, MILLISECONDS).get();
   }
 
+  private static <T> Supplier<T> memoizeWithExpiration(Supplier<T> s, long duration,
+      TimeUnit units) {
+    // This insanity exists to make modernizer plugin happy. We are living in the future now.
+    return () -> Suppliers.memoizeWithExpiration(s::get, duration, units).get();
+  }
+

Review Comment:
   I am not sure this is actually needed. Also, the comment isn't very helpful. I was able to inline this easily and use a static import, and completely remove this method, as well as the method it was copied from. Modernizer is perfectly fine with it.



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org