Enable adaptive refresh by default #3249 (#3316)

* Enable adaptive refresh by default #3249

* Added @SInCE

* Fix integration test [incomplete]

* Add also fix to issue 3250

* Change default to 5 seconds

* Remove unnecessary new line

Author: tishun

src/main/java/io/lettuce/core/cluster/ClusterClientOptions.java

@@ -53,7 +53,8 @@ public class ClusterClientOptions extends ClientOptions {
 
     public static final Duration DEFAULT_REFRESH_PERIOD_DURATION = Duration.ofSeconds(DEFAULT_REFRESH_PERIOD);
 
-    public static final boolean DEFAULT_VALIDATE_CLUSTER_MEMBERSHIP = true;
+    /** Since Lettuce 7.0 validation is by default disabled. */
+    public static final boolean DEFAULT_VALIDATE_CLUSTER_MEMBERSHIP = false;
 
     public static final Predicate<RedisClusterNode> DEFAULT_NODE_FILTER = node -> true;
 
@@ -281,8 +282,12 @@ public Builder topologyRefreshOptions(ClusterTopologyRefreshOptions topologyRefr
         }
 
         /**
-         * Validate the cluster node membership before allowing connections to a cluster node. Defaults to {@code true}. See
+         * Validate the cluster node membership before allowing connections to a cluster node. Defaults to {@code false}. See
          * {@link ClusterClientOptions#DEFAULT_VALIDATE_CLUSTER_MEMBERSHIP}.
+         * <p/>
+         * Since 7.0, validation is disabled by default, as it is causing problems in some upgrade scenarios. In scenarios where
+         * upgraded nodes are added to the cluster the ASK / MOVED replies usually come before the topology is refreshed and -
+         * respectively - this validation would fail.
          *
          * @param validateClusterNodeMembership {@code true} if validation is enabled.
          * @return {@code this}
@@ -396,7 +401,7 @@ public ClusterTopologyRefreshOptions getTopologyRefreshOptions() {
     }
 
     /**
-     * Validate the cluster node membership before allowing connections to a cluster node. Defaults to {@code true}.
+     * Validate the cluster node membership before allowing connections to a cluster node. Defaults to {@code false}.
      *
      * @return {@code true} if validation is enabled.
      */

src/main/java/io/lettuce/core/cluster/ClusterTopologyRefreshOptions.java

@@ -33,14 +33,18 @@
  * Options to control the Cluster topology refreshing of {@link RedisClusterClient}.
  *
  * @author Mark Paluch
+ * @author Tihomir Mateev
  * @since 4.2
  */
 public class ClusterTopologyRefreshOptions {
 
-    public static final Set<RefreshTrigger> DEFAULT_ADAPTIVE_REFRESH_TRIGGERS = Collections.emptySet();
+    /** Since Lettuce 7.0 all adaptive triggers are enabled by default */
+    public static final Set<RefreshTrigger> DEFAULT_ADAPTIVE_REFRESH_TRIGGERS = EnumSet.allOf(RefreshTrigger.class);
 
-    public static final long DEFAULT_ADAPTIVE_REFRESH_TIMEOUT = 30;
+    /** Since Lettuce 7.0 the default adaptive refresh timeout is 5 seconds */
+    public static final long DEFAULT_ADAPTIVE_REFRESH_TIMEOUT = 5;
 
+    @Deprecated
     public static final TimeUnit DEFAULT_ADAPTIVE_REFRESH_TIMEOUT_UNIT = TimeUnit.SECONDS;
 
     public static final Duration DEFAULT_ADAPTIVE_REFRESH_TIMEOUT_DURATION = Duration
@@ -54,6 +58,7 @@ public class ClusterTopologyRefreshOptions {
 
     public static final long DEFAULT_REFRESH_PERIOD = 60;
 
+    @Deprecated
     public static final TimeUnit DEFAULT_REFRESH_PERIOD_UNIT = TimeUnit.SECONDS;
 
     public static final Duration DEFAULT_REFRESH_PERIOD_DURATION = Duration.ofSeconds(DEFAULT_REFRESH_PERIOD);
@@ -159,12 +164,16 @@ private Builder() {
          * Enables adaptive topology refreshing using one or more {@link RefreshTrigger triggers}. Adaptive refresh triggers
          * initiate topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
          * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
-         * a large scale. Adaptive refresh triggers are disabled by default. See also
+         * a large scale. Adaptive refresh triggers are all enabled by default. See also
          * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
          *
          * @param refreshTrigger one or more {@link RefreshTrigger} to enabled
          * @return {@code this}
+         * @deprecated Starting from 7.0, this method has no effect as all adaptive triggers are enabled by default.
+         * @see #disableAllAdaptiveRefreshTriggers()
+         * @see #disableAdaptiveRefreshTrigger(RefreshTrigger...)
          */
+        @Deprecated
         public Builder enableAdaptiveRefreshTrigger(RefreshTrigger... refreshTrigger) {
 
             LettuceAssert.notNull(refreshTrigger, "RefreshTriggers must not be null");
@@ -179,16 +188,56 @@ public Builder enableAdaptiveRefreshTrigger(RefreshTrigger... refreshTrigger) {
          * Enables adaptive topology refreshing using all {@link RefreshTrigger triggers}. Adaptive refresh triggers initiate
          * topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
          * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
-         * a large scale. Adaptive refresh triggers are disabled by default. See also
+         * a large scale. Adaptive refresh triggers are all enabled by default. See also
          * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
          *
          * @return {@code this}
+         * @deprecated Starting from 7.0, this method has no effect as all adaptive triggers are enabled by default.
+         * @see #disableAllAdaptiveRefreshTriggers()
+         * @see #disableAdaptiveRefreshTrigger(RefreshTrigger...)
          */
+        @Deprecated
         public Builder enableAllAdaptiveRefreshTriggers() {
             adaptiveRefreshTriggers.addAll(EnumSet.allOf(RefreshTrigger.class));
             return this;
         }
 
+        /**
+         * Disables adaptive topology refreshing using one or more {@link RefreshTrigger triggers}. Adaptive refresh triggers
+         * initiate topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
+         * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
+         * a large scale. Adaptive refresh triggers are all enabled by default. See also
+         * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
+         *
+         * @param refreshTrigger one or more {@link RefreshTrigger} to enabled
+         * @return {@code this}
+         * @since 7.0
+         */
+        public Builder disableAdaptiveRefreshTrigger(RefreshTrigger... refreshTrigger) {
+
+            LettuceAssert.notNull(refreshTrigger, "RefreshTriggers must not be null");
+            LettuceAssert.noNullElements(refreshTrigger, "RefreshTriggers must not contain null elements");
+            LettuceAssert.notEmpty(refreshTrigger, "RefreshTriggers must contain at least one element");
+
+            Arrays.asList(refreshTrigger).forEach(adaptiveRefreshTriggers::remove);
+            return this;
+        }
+
+        /**
+         * Disables adaptive topology refreshing using all {@link RefreshTrigger triggers}. Adaptive refresh triggers initiate
+         * topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
+         * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
+         * a large scale. Adaptive refresh triggers are all enabled by default. See also
+         * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
+         *
+         * @return {@code this}
+         * @since 7.0
+         */
+        public Builder disableAllAdaptiveRefreshTriggers() {
+            adaptiveRefreshTriggers.clear();
+            return this;
+        }
+
         /**
          * Set the timeout for adaptive topology updates. This timeout is to rate-limit topology updates initiated by refresh
          * triggers to one topology refresh per timeout. Defaults to {@literal 30 SECONDS}. See {@link #DEFAULT_REFRESH_PERIOD}

src/test/java/io/lettuce/core/cluster/ClusterTopologyRefreshOptionsUnitTests.java

@@ -25,6 +25,7 @@ void testBuilder() {
         ClusterTopologyRefreshOptions options = ClusterTopologyRefreshOptions.builder()//
                 .enablePeriodicRefresh(true).refreshPeriod(10, TimeUnit.MINUTES)//
                 .dynamicRefreshSources(false) //
+                .disableAllAdaptiveRefreshTriggers()//
                 .enableAdaptiveRefreshTrigger(RefreshTrigger.MOVED_REDIRECT)//
                 .adaptiveRefreshTriggersTimeout(15, TimeUnit.MILLISECONDS)//
                 .closeStaleConnections(false)//
@@ -46,6 +47,7 @@ void testCopy() {
         ClusterTopologyRefreshOptions master = ClusterTopologyRefreshOptions.builder()//
                 .enablePeriodicRefresh(true).refreshPeriod(10, TimeUnit.MINUTES)//
                 .dynamicRefreshSources(false) //
+                .disableAllAdaptiveRefreshTriggers()//
                 .enableAdaptiveRefreshTrigger(RefreshTrigger.MOVED_REDIRECT)//
                 .adaptiveRefreshTriggersTimeout(15, TimeUnit.MILLISECONDS)//
                 .closeStaleConnections(false)//

src/test/java/io/lettuce/core/cluster/ClusterTopologyRefreshSchedulerUnitTests.java

@@ -132,7 +132,7 @@ void shouldTriggerRefreshOnAskRedirection() {
     }
 
     @Test
-    void shouldNotTriggerAdaptiveRefreshUsingDefaults() {
+    void shouldTriggerAdaptiveRefreshUsingDefaults() {
 
         ClusterTopologyRefreshOptions clusterTopologyRefreshOptions = ClusterTopologyRefreshOptions.create();
 
@@ -141,6 +141,21 @@ void shouldNotTriggerAdaptiveRefreshUsingDefaults() {
 
         when(clusterClient.getClusterClientOptions()).thenReturn(clusterClientOptions);
 
+        sut.onAskRedirection();
+        verify(eventExecutors).submit(any(Runnable.class));
+    }
+
+    @Test
+    void shouldNotTriggerAdaptiveRefreshWhenDisabled() {
+
+        ClusterTopologyRefreshOptions clusterTopologyRefreshOptions = ClusterTopologyRefreshOptions.builder()
+                .disableAllAdaptiveRefreshTriggers().build();
+
+        ClusterClientOptions clusterClientOptions = ClusterClientOptions.builder()
+                .topologyRefreshOptions(clusterTopologyRefreshOptions).build();
+
+        when(clusterClient.getClusterClientOptions()).thenReturn(clusterClientOptions);
+
         sut.onAskRedirection();
         verify(eventExecutors, never()).submit(any(Runnable.class));
     }