[Flaky-test] Fix metadata cache flaky tests

[Demogorgon314]

Motivation

The MetadataCache#getIfCached method will return Optional.empty() if the required path is not cached yet or the cached future is not finished yet, this is normal behavior.

pulsar/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java

Lines 130 to 137 in 773f919

	public Optional<T> getIfCached(String path) {
	CompletableFuture<Optional<CacheGetResult<T>>> future = objCache.getIfPresent(path);
	if (future != null && future.isDone() && !future.isCompletedExceptionally()) {
	return future.join().map(CacheGetResult::getValue);
	} else {
	return Optional.empty();
	}
	}

We should add a retry mechanism when asserting getIfCached.

Modifications

Add a retry mechanism to assert getIfCached.

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

• doc-required

  (If you need help on updating docs, create a doc issue)

• no-need-doc

  (Please explain why)

• doc

  (If this PR contains doc changes)

[Jason918]

LGTM

[lhotari]

@Demogorgon314 We should use Awaitility instead of expanding the use of "retryStrategically" which exists in some older test code. Was there a specific reason to not use Awaitility in this PR?

[Demogorgon314]

@lhotari
In this PR, we need ensure objCache.getIfCached(key1) only return Optional.empty() or correct value.

If we use Awaitility.await().untilAsserted(...); here, when objCache.getIfCached(key1) return a wrong value it still retry.

Awaitility.await().untilAsserted(() -> {
    assertEquals(objCache.getIfCached(key1), Optional.of(v));
});

If Awaitility can do what I said, please tell me. Thanks! :-)

[lhotari]

@lhotari In this PR, we need ensure objCache.getIfCached(key1) only return Optional.empty() or correct value.

If we use Awaitility.await().untilAsserted(...); here, when objCache.getIfCached(key1) return a wrong value it still retry.

Awaitility.await().untilAsserted(() -> {
    assertEquals(objCache.getIfCached(key1), Optional.of(v));
});

If Awaitility can do what I said, please tell me. Thanks! :-)

I don't see a reason why Awaitility couldn't handle this. Sometimes it's necessary to add .ignoreExceptions() to the Awaitility configuration. This is needed if the assertion throws some other exception than an AssertionError.

Did you try

 Awaitility.await().ignoreExceptions().untilAsserted(() -> {
     assertEquals(objCache.getIfCached(key1), Optional.of(v));
 });

I'm also wondering the logic in retryStrategically. It looks like the check will always pass when i == (retryCount - 1). Perhaps that's the reason why it "works"? @Demogorgon314 please check

pulsar/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java

Lines 611 to 620 in 8264414

	public static boolean retryStrategically(Predicate<Void> predicate, int retryCount, long intSleepTimeInMillis)
	throws Exception {
	for (int i = 0; i < retryCount; i++) {
	if (predicate.test(null) || i == (retryCount - 1)) {
	return true;
	}
	Thread.sleep(intSleepTimeInMillis + (intSleepTimeInMillis * i));
	}
	return false;
	}

[lhotari]

@Demogorgon314 I made a PR to address the issue, it's #14518 . Please review it.

[lhotari]

I closed #14518 since I came to the conclusion that this PR #14373 should be reverted instead.
Besides the retryStrategically being invalid (it will always pass on the last retry), this type of code doesn't make sense to me:

pulsar/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java

Lines 272 to 273 in 8264414

	assertEquals(objCache.get(key1).join(), Optional.of(value2));
	assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(value2), Optional.empty());

[Demogorgon314]

@lhotari
Yes, we should remove i == (retryCount - 1) check.