ehcache reconnect changes by mobasherul-ibm · Pull Request #3261 · ehcache/ehcache3

mobasherul-ibm · 2025-05-14T08:25:55Z

No description provided.

...ache-client/src/main/java/org/ehcache/clustered/client/internal/service/ConnectionState.java

...nt/src/main/java/org/ehcache/clustered/client/internal/service/DefaultClusteringService.java

.../org/ehcache/clustered/client/internal/reconnect/FailedReconnectClusterTierClientEntity.java

...on-test/src/test/java/org/ehcache/clustered/reconnect/CacheStateRepositoryReconnectTest.java

cljohnso · 2025-05-20T18:28:09Z

...ered/integration-test/src/test/java/org/ehcache/clustered/reconnect/EventsReconnectTest.java

        fail();
      } catch (ExecutionException e) {
-        assertThat(e.getCause().getCause().getCause(), instanceOf(ReconnectInProgressException.class));
+        assertThat(e.getCause().getCause().getCause().getCause(), instanceOf(ReconnectInProgressException.class));


Additional exception nesting ... this is a behavior change and should be reconsidered.

cljohnso · 2025-05-20T18:29:12Z

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/ThrowingResiliencyStrategy.java

  @Override
  public void putFailure(K key, V value, StoreAccessException e) {
-    if (e.getCause() instanceof ReconnectInProgressException) {
+    if (e.getCause().getCause() instanceof ReconnectInProgressException || e.getCause() instanceof ReconnectInProgressException) {


The additional level of exception testing bothers me. Can it be avoided?

...tered/integration-test/src/test/java/org/ehcache/clustered/IterationFailureBehaviorTest.java

chrisdennis

The changes to the exception handling here seem like they might be amplifying and obfuscating these changes. If we can manage to preserve much of the original exception throwing/logic I feel we'll have a smaller more trustable and reviewable changeset.

.../integration-test/src/test/java/org/ehcache/clustered/reconnect/BasicCacheReconnectTest.java

chrisdennis · 2025-05-20T19:54:06Z

.../integration-test/src/test/java/org/ehcache/clustered/reconnect/BasicCacheReconnectTest.java

      });

-      getSucceededFuture.get(20000, TimeUnit.MILLISECONDS);
+      assertThat(getSucceededFuture::isDone, eventually().is(true));


This squelches any exceptions throw by the underlying task.

didn't get that exception will be thrown for some time since reconnection is happening in background. I changed this since it can be slow at times for reconnect to complete. Eventually will still throw the timeout exception as it waits for 1 min for the future to succeed.

One of us is misunderstanding the other... isDone just means complete, either exceptionally or normally. If it completed exceptionally we probably want to know what the exception is.

...nt/src/main/java/org/ehcache/clustered/client/internal/service/DefaultClusteringService.java

mobasherul-ibm · 2025-05-21T05:20:08Z

The changes to the exception handling here seem like they might be amplifying and obfuscating these changes. If we can manage to preserve much of the original exception throwing/logic I feel we'll have a smaller more trustable and reviewable changeset.

Earlier flow was store -> reconnectingProxy -> commonProxy -> actualentity where exception flow was -
(StoreAccessException <- ReconnectInProgressException) when connection breaks Or for other scenarios
(StoreAccessException<-ServerStoreProxyException<-SomeOtherException) because commonProxy wraps ConnectionClosedException with ServerStoreProxyException and that is passed to reconnectingProxy which just throws ReconnectInProgressException.

Since I removed reconnectingProxy and introduced reconnectable entity flow has changed to -
store -> commonProxy -> reconnectableentity -> actual entity where exception flow has changed to -
(StoreAccessException <- ServerStoreProxyException <- ReconnectInProgressException) when connection breaks as commonProxy is now wrapping the ReconnectInProgressException with ServerStoreProxyException and for other exception it will be -
(StoreAccessException <-. ServerStoreProxyException <- SomeOtherException).

cljohnso · 2025-05-28T19:57:55Z

@mobasherul-ibm please do not resolve comments you did not create -- it makes re-review considerably more difficult. If you want to keep track of items you've addressed, reply to the comment with a +1 or a :white_check_mark, even a simple done.

.../org/ehcache/clustered/client/internal/reconnect/FailedReconnectClusterTierClientEntity.java

...src/main/java/org/ehcache/clustered/client/internal/store/SimpleClusterTierClientEntity.java

...hcache-client/src/test/java/org/ehcache/clustered/client/internal/service/ReconnectTest.java

...va/org/ehcache/clustered/client/internal/reconnect/ReconnectableClusterTierClientEntity.java

...tered/integration-test/src/test/java/org/ehcache/clustered/IterationFailureBehaviorTest.java

cljohnso · 2025-05-28T20:23:43Z

Still have a test failure in org.ehcache.clustered.IterationFailureBehaviorTest#testIteratorReconnect

...va/org/ehcache/clustered/client/internal/reconnect/ReconnectableClusterTierClientEntity.java

...tered/integration-test/src/test/java/org/ehcache/clustered/IterationFailureBehaviorTest.java

...va/org/ehcache/clustered/client/internal/reconnect/ReconnectableClusterTierClientEntity.java

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/MultipleCacheReconnectTest.java

cljohnso · 2025-06-09T22:16:37Z

...t/src/test/java/org/ehcache/clustered/replication/BasicClusteredCacheOpsReplicationTest.java

  }

  @Test
+  @Ignore


Is this being ignored with expectation for a future repair? If so, an issue number should be mentioned.

cljohnso · 2025-06-09T22:16:45Z

...va/org/ehcache/clustered/replication/BasicClusteredCacheOpsReplicationMultiThreadedTest.java

  }

  @Test
+  @Ignore


Is this being ignored with expectation for a future repair? If so, an issue number should be mentioned.

cljohnso · 2025-06-09T22:17:08Z

clustered/integration-test/build.gradle

  systemProperty 'kitInstallationPath', "$unzipKit.destinationDir/${project(':clustered:ehcache-clustered').archivesBaseName}-$project.version-kit"
  // Uncomment to include client logging in console output
-  // testLogging.showStandardStreams = true
+   testLogging.showStandardStreams = true


Permanent change?

will remove once approved

chrisdennis · 2025-06-10T21:10:41Z

...tered/integration-test/src/test/java/org/ehcache/clustered/IterationFailureBehaviorTest.java

      = CacheManagerBuilder.newCacheManagerBuilder()
      .with(ClusteringServiceConfigurationBuilder.cluster(CLUSTER.get().getConnectionURI().resolve("/iterator-cm"))
-        .autoCreate(server -> server.defaultServerResource("primary-server-resource")));
+        .autoCreateOnReconnect(server -> server.defaultServerResource("primary-server-resource")));


I follow the logic of why this should need to be autoCreateOnReconnect... what I don't get is how we were getting away with it before. What changed?

earlier entity were not reconnectable so there were no connection retries but now that changed thats why I changed it to ensure all connections are closed at the end of this test.

cljohnso

Needs conversations with Chris and possibly some minor tweaks.

cljohnso · 2025-06-30T17:33:54Z

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/MultipleCacheReconnectTest.java

+        cache2.put(0L, "secondcache");
+      });
+
+      assertThat(() -> putFuture1.get(5000, TimeUnit.MILLISECONDS),


Is this going to need an eventually to have a stable test?

cljohnso · 2025-06-30T17:34:10Z

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/MultipleCacheReconnectTest.java

+        threw(Matchers.<Throwable>both(instanceOf(ExecutionException.class))
+          .and(hasCause(hasCause(hasCause(hasCause(instanceOf(ReconnectInProgressException.class))))))));
+
+      assertThat(() -> putFuture2.get(5000, TimeUnit.MILLISECONDS),


Is this going to need an eventually to have a stable test?

cljohnso · 2025-06-30T17:35:54Z

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/MultipleCacheReconnectTest.java

+      cache2.put(0L, "secondValue");
+      expireLease();
+
+      assertThat(() -> cache1.get(0L),


Is it possible the reconnect succeeds before this statement runs?

cljohnso · 2025-06-30T17:37:31Z

...tegration-test/src/test/java/org/ehcache/clustered/reconnect/MultipleCacheReconnectTest.java

+
+      cacheManager.removeCache("clustered-cache1");
+
+      try {


This would be better with the assertThat(..., threw(...)) pattern.

mobasherul-ibm force-pushed the state_repo_reconnect branch 5 times, most recently from 4550847 to 891ddc3 Compare May 16, 2025 12:54

mobasherul-ibm requested review from chrisdennis and cljohnso May 16, 2025 12:54

mobasherul-ibm force-pushed the state_repo_reconnect branch 2 times, most recently from 159d9b5 to 71eb82d Compare May 19, 2025 04:51

cljohnso requested changes May 20, 2025

View reviewed changes

chrisdennis requested changes May 20, 2025

View reviewed changes

mobasherul-ibm force-pushed the state_repo_reconnect branch 3 times, most recently from cba165e to 168f777 Compare May 21, 2025 14:29

mobasherul-ibm requested review from chrisdennis and cljohnso May 21, 2025 14:31

mobasherul-ibm force-pushed the state_repo_reconnect branch 2 times, most recently from 22fde53 to 3066706 Compare May 21, 2025 18:56

cljohnso requested changes May 28, 2025

View reviewed changes

mobasherul-ibm force-pushed the state_repo_reconnect branch 5 times, most recently from 662e6bb to d7a7a21 Compare June 5, 2025 18:08

mobasherul-ibm requested a review from cljohnso June 6, 2025 06:18

mobasherul-ibm force-pushed the state_repo_reconnect branch from d7a7a21 to 6ca13df Compare June 6, 2025 07:02

cljohnso requested changes Jun 9, 2025

View reviewed changes

mobasherul-ibm force-pushed the state_repo_reconnect branch from 6ca13df to b16d57c Compare June 10, 2025 05:08

mobasherul-ibm requested a review from cljohnso June 10, 2025 05:15

mobasherul-ibm force-pushed the state_repo_reconnect branch from b16d57c to 4210c45 Compare June 10, 2025 05:41

chrisdennis reviewed Jun 10, 2025

View reviewed changes

Fix ehcache reconnect

abf7e59

mobasherul-ibm force-pushed the state_repo_reconnect branch from 4210c45 to abf7e59 Compare June 13, 2025 06:06

mobasherul-ibm requested a review from chrisdennis June 13, 2025 06:07

cljohnso reviewed Jun 30, 2025

View reviewed changes

                 }
                 @Test
+                @Ignore

Conversation

mobasherul-ibm commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisdennis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mobasherul-ibm commented May 21, 2025

Uh oh!

cljohnso commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cljohnso commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdennis Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cljohnso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

cljohnso commented May 28, 2025 •

edited

Loading

chrisdennis Jun 10, 2025 •

edited

Loading