GEODE-9617: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size #6976

mhansonp · 2021-10-11T22:22:25Z

Did some code cleanup up and made a change to Latch calls to check return values.

mhansonp · 2021-10-11T22:24:19Z

...istributedTest/java/org/apache/geode/internal/cache/PartitionedRegionSingleHopDUnitTest.java


-    LATCH.get().await(getTimeout().toMillis(), MILLISECONDS);
+    assertThat(LATCH.get().await(getTimeout().toMillis(), MILLISECONDS)).isTrue();


important change. A timeout on a latch is possible and will return false.

mhansonp · 2021-10-11T22:24:41Z

...istributedTest/java/org/apache/geode/internal/cache/PartitionedRegionSingleHopDUnitTest.java

@@ -952,16 +915,16 @@ public void testClientMetadataForPersistentPrs() throws Exception {
      });
    }

-    LATCH.get().await(getTimeout().toMillis(), MILLISECONDS);
+    assertThat(LATCH.get().await(getTimeout().toMillis(), MILLISECONDS)).isTrue();


important change. A timeout on a latch is possible and will return false.

I may be misunderstanding something here, but am I right in thinking that this change won't prevent the test from failing, just make it fail at a different point than described in GEODE-9617? If the failure described in GEODE-9617 is due to this await silently timing out, then asserting here that it doesn't time out will just move the failure to here.

Also, GEODE-9617 seems to describe two distinct failures; one in this test, and one in testMetadataServiceCallAccuracy_FromGetOp(). Since this PR doesn't modify testMetadataServiceCallAccuracy_FromGetOp() beyond minor refactoring, is it accurate to say that any flakiness in that test won't be affected by it?

Finally, could the title of this PR be changed to meet the criteria described on the wiki: https://cwiki.apache.org/confluence/display/GEODE/Commit+Message+Format

These changes to nothing to affect that. That will be a separate PR.

Though that does not appear to reproduce at this time...

mhansonp · 2021-10-12T01:18:17Z

Seems like the latch changes made the sticking point a little more obvious...

kirklund

I know it's still in DRAFT, but it looks good to me!

onichols-pivotal · 2022-02-05T08:05:54Z

this PR appears to be abandoned, can it be closed?

onichols-pivotal

comment deleted

- found a bug in my changes.

mhansonp · 2022-05-11T23:56:19Z

...istributedTest/java/org/apache/geode/internal/cache/PartitionedRegionSingleHopDUnitTest.java

@@ -956,16 +935,16 @@ public void testClientMetadataForPersistentPrs() throws Exception {
      });
    }

-    LATCH.get().await(getTimeout().toMillis(), MILLISECONDS);
-
+    assertThat(LATCH.get().await(getTimeout().toMillis(), MILLISECONDS))


Another latch change

DonalEvans

Per the commit message format guidelines found on the wiki, could this PR name (or the final commit message when merging) be changed to describe what the commit is actually changing rather than just being a restatement of the ticket title? Maybe something like "GEODE-9617: Assert on successful latch await", since as far as I can see, none of the changes are intended to address the flaky test, just make it more obvious when it does fail?

DonalEvans · 2022-05-13T16:36:28Z

...istributedTest/java/org/apache/geode/internal/cache/PartitionedRegionSingleHopDUnitTest.java

 public class PartitionedRegionSingleHopDUnitTest implements Serializable {
-
+  protected static Logger logger = LogService.getLogger();


This logger is not used in the test so should be removed.

Great! Thanks!

...istributedTest/java/org/apache/geode/internal/cache/PartitionedRegionSingleHopDUnitTest.java

DonalEvans · 2022-05-13T16:51:44Z

geode-core/src/distributedTest/java/org/apache/geode/cache30/PutAllMultiVmDUnitTest.java


-    vm0.invoke(new CacheSerializableRunnable("testSimplePutAll1") {
+    vm0.invoke("testSimplePutAll1", new CacheSerializableRunnable() {


This (and other uses of CacheSerializableRunnable below) can be simplified by using a lambda instead:

vm0.invoke("testSimplePutAll1", () -> { int cntr = 0, cntr1 = 0; for (int i = 1; i < 6; i++) { region.put(i, "testSimplePutAll" + i); cntr++; } ... });

You cannot do that because CacheSerializableRunnable throws a CacheException

@Override public void run() { try { run2(); } catch (CacheException exception) { String message = "While invoking \"" + this + "\""; throw new CacheSerializableRunnableException(message, exception); } }

geode-core/src/distributedTest/java/org/apache/geode/cache30/PutAllMultiVmDUnitTest.java

mhansonp requested review from DonalEvans, jchen21, kirklund and nabarunnag as code owners October 11, 2021 22:22

mhansonp commented Oct 11, 2021

View reviewed changes

mhansonp marked this pull request as draft October 12, 2021 01:17

kirklund approved these changes Oct 12, 2021

View reviewed changes

onichols-pivotal requested changes Feb 5, 2022

View reviewed changes

onichols-pivotal approved these changes Feb 25, 2022

View reviewed changes

mhansonp force-pushed the bugfix/GEODE-9617 branch from 76dda7f to 27cad91 Compare April 20, 2022 17:01

mhansonp force-pushed the bugfix/GEODE-9617 branch from 6c873b1 to ba5120a Compare May 5, 2022 18:59

GEODE-9617: a bit of cleanup

ba67c34

mhansonp force-pushed the bugfix/GEODE-9617 branch from f1ab552 to 305c6a9 Compare May 6, 2022 22:36

mhansonp marked this pull request as ready for review May 11, 2022 21:30

GEODE-9617: removing some logging I left in.

7d27fa4

- found a bug in my changes.

mhansonp force-pushed the bugfix/GEODE-9617 branch from 305c6a9 to 7d27fa4 Compare May 11, 2022 21:36

mhansonp changed the title ~~Bugfix/GEODE-9617~~ Bugfix/GEODE-9617 CI Failure: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size May 11, 2022

mhansonp commented May 11, 2022

View reviewed changes

DonalEvans reviewed May 13, 2022

View reviewed changes

GEODE-9617: Code review cleanup

573a075

mhansonp closed this May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-9617: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size #6976

GEODE-9617: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size #6976

mhansonp commented Oct 11, 2021

mhansonp Oct 11, 2021

mhansonp Oct 11, 2021

DonalEvans Oct 11, 2021

mhansonp May 12, 2022

mhansonp May 12, 2022

mhansonp commented Oct 12, 2021

kirklund left a comment •

edited

onichols-pivotal commented Feb 5, 2022

onichols-pivotal left a comment •

edited

mhansonp May 11, 2022

DonalEvans left a comment

DonalEvans May 13, 2022

mhansonp May 13, 2022

DonalEvans May 13, 2022

mhansonp May 16, 2022


		LATCH.get().await(getTimeout().toMillis(), MILLISECONDS);
		assertThat(LATCH.get().await(getTimeout().toMillis(), MILLISECONDS)).isTrue();

		public class PartitionedRegionSingleHopDUnitTest implements Serializable {

		protected static Logger logger = LogService.getLogger();


		vm0.invoke(new CacheSerializableRunnable("testSimplePutAll1") {
		vm0.invoke("testSimplePutAll1", new CacheSerializableRunnable() {

GEODE-9617: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size #6976

GEODE-9617: PartitionedRegionSingleHopDUnitTest fails with ConditionTimeoutException waiting for server to bucket map size #6976

Conversation

mhansonp commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhansonp commented Oct 12, 2021

kirklund left a comment • edited

Choose a reason for hiding this comment

onichols-pivotal commented Feb 5, 2022

onichols-pivotal left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirklund left a comment •

edited

onichols-pivotal left a comment •

edited