You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/02/18 09:54:36 UTC
[GitHub] [pulsar] Demogorgon314 opened a new pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Demogorgon314 opened a new pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373
### 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.
https://github.com/apache/pulsar/blob/773f9197cfb83092fb6ee2bf1b9ec38143f3c8b0/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java#L130-L137
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)
- [x] `no-need-doc`
(Please explain why)
- [ ] `doc`
(If this PR contains doc changes)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055546344
@lhotari
Yes, we should remove `i == (retryCount - 1)` check.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055476049
@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.
```java
Awaitility.await().untilAsserted(() -> {
assertEquals(objCache.getIfCached(key1), Optional.of(v));
});
```
If `Awaitility` can do what I said, please tell me. Thanks! :-)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a change in pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#discussion_r813505145
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -233,7 +234,7 @@ public void insertionDeletionWitGenericType(String provider, Supplier<String> ur
v.put("b", "2");
objCache.create(key1, v).join();
- assertEquals(objCache.getIfCached(key1), Optional.of(v));
+ assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(v), Optional.empty());
Review comment:
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.
```java
Awaitility.await().untilAsserted(() -> {
assertEquals(objCache.getIfCached(key1), Optional.of(v));
});
```
If `Awaitility` can do what I said, please tell me. Thanks!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a change in pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#discussion_r809868714
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -233,7 +234,7 @@ public void insertionDeletionWitGenericType(String provider, Supplier<String> ur
v.put("b", "2");
objCache.create(key1, v).join();
- assertEquals(objCache.getIfCached(key1), Optional.of(v));
+ assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(v), Optional.empty());
Review comment:
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.
```java
Awaitility.await().untilAsserted(() -> {
assertEquals(objCache.getIfCached(key1), Optional.of(v));
});
```
If `Awaitility` can do what I said, please tell me. Thanks!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#discussion_r809848403
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -233,7 +234,7 @@ public void insertionDeletionWitGenericType(String provider, Supplier<String> ur
v.put("b", "2");
objCache.create(key1, v).join();
- assertEquals(objCache.getIfCached(key1), Optional.of(v));
+ assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(v), Optional.empty());
Review comment:
Why not use `Awaitility.await().untilAsserted(...);`?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055344353
@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?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055500489
> @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.
>
> ```java
> 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
```java
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"?
https://github.com/apache/pulsar/blob/8264414547cd5e837c0c8a625d7346c03353b43a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java#L611-L620
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#discussion_r809959999
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -233,7 +234,7 @@ public void insertionDeletionWitGenericType(String provider, Supplier<String> ur
v.put("b", "2");
objCache.create(key1, v).join();
- assertEquals(objCache.getIfCached(key1), Optional.of(v));
+ assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(v), Optional.empty());
Review comment:
> 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.
>
> ```java
> Awaitility.await().untilAsserted(() -> {
> assertEquals(objCache.getIfCached(key1), Optional.of(v));
> });
> ```
>
> If `Awaitility` can do what I said, please tell me. Thanks!
Yes, it is.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055344353
@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 now use Awaitility in this PR?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055500489
> @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.
>
> ```java
> 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
```java
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
https://github.com/apache/pulsar/blob/8264414547cd5e837c0c8a625d7346c03353b43a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java#L611-L620
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055524500
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:
https://github.com/apache/pulsar/blob/8264414547cd5e837c0c8a625d7346c03353b43a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java#L272-L273
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055524500
I closed #14518 since I came to the conclusion that this PR #14373 should be reverted instead.
Besides the retryStrategically being invalid, this type of code doesn't make sense to me:
https://github.com/apache/pulsar/blob/8264414547cd5e837c0c8a625d7346c03353b43a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java#L272-L273
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#issuecomment-1055517390
@Demogorgon314 I made a PR to address the issue, it's #14518 . Please review 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373#discussion_r813470398
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -233,7 +234,7 @@ public void insertionDeletionWitGenericType(String provider, Supplier<String> ur
v.put("b", "2");
objCache.create(key1, v).join();
- assertEquals(objCache.getIfCached(key1), Optional.of(v));
+ assertEqualsAndRetry(() -> objCache.getIfCached(key1), Optional.of(v), Optional.empty());
Review comment:
Can we use the Awaitibility to instread?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14373: [Flaky-test] Fix metadata cache flaky tests
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14373:
URL: https://github.com/apache/pulsar/pull/14373
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org