You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "dao-jun (via GitHub)" <gi...@apache.org> on 2024/03/12 19:35:11 UTC
[PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
dao-jun opened a new pull request, #22256:
URL: https://github.com/apache/pulsar/pull/22256
Fixes https://github.com/apache/pulsar/issues/20495
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository:
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun closed pull request #22256: [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed
URL: https://github.com/apache/pulsar/pull/22256
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1522283656
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1235,22 +1235,24 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
});
if (!pulsar.getConfiguration().isEnableNonPersistentTopics()) {
if (log.isDebugEnabled()) {
log.debug("Broker is unable to load non-persistent topic {}", topic);
}
- return FutureUtil.failedFuture(
+ topicFuture.completeExceptionally(
new NotAllowedException("Broker is not unable to load non-persistent topic"));
+ return topicFuture;
}
final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
NonPersistentTopic nonPersistentTopic;
try {
nonPersistentTopic = newTopic(topic, null, this, NonPersistentTopic.class);
} catch (Throwable e) {
log.warn("Failed to create topic {}", topic, e);
- return FutureUtil.failedFuture(e);
+ topicFuture.completeExceptionally(e);
+ return topicFuture;
Review Comment:
Oh yes, got it now.
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1522274998
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1235,22 +1235,24 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
});
if (!pulsar.getConfiguration().isEnableNonPersistentTopics()) {
if (log.isDebugEnabled()) {
log.debug("Broker is unable to load non-persistent topic {}", topic);
}
- return FutureUtil.failedFuture(
+ topicFuture.completeExceptionally(
new NotAllowedException("Broker is not unable to load non-persistent topic"));
+ return topicFuture;
}
final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
NonPersistentTopic nonPersistentTopic;
try {
nonPersistentTopic = newTopic(topic, null, this, NonPersistentTopic.class);
} catch (Throwable e) {
log.warn("Failed to create topic {}", topic, e);
- return FutureUtil.failedFuture(e);
+ topicFuture.completeExceptionally(e);
+ return topicFuture;
Review Comment:
This actually supplements the handling of abnormal branches
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#issuecomment-1992727753
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/22256?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `88.23529%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 73.59%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ed38328`)](https://app.codecov.io/gh/apache/pulsar/pull/22256?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 40 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/22256?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #22256 +/- ##
============================================
+ Coverage 73.57% 73.59% +0.02%
+ Complexity 32624 32250 -374
============================================
Files 1877 1879 +2
Lines 139502 139796 +294
Branches 15299 15335 +36
============================================
+ Hits 102638 102883 +245
- Misses 28908 28926 +18
- Partials 7956 7987 +31
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/22256/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/22256/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.81% <23.52%> (+2.22%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/22256/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.33% <0.00%> (+<0.01%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/22256/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.86% <88.23%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pulsar/pull/22256?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://app.codecov.io/gh/apache/pulsar/pull/22256?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `98.38% <100.00%> (ø)` | |
| [...rg/apache/pulsar/broker/service/BrokerService.java](https://app.codecov.io/gh/apache/pulsar/pull/22256?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `81.24% <87.50%> (+0.45%)` | :arrow_up: |
... and [76 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/22256/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#issuecomment-1994507204
@lhotari @poorbarcode Could you plz take a look?
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun closed pull request #22256: [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed
URL: https://github.com/apache/pulsar/pull/22256
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1523883392
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1637,10 +1639,9 @@ private void createPersistentTopic(final String topic, boolean createIfMissing,
Map<String, String> properties, @Nullable TopicPolicies topicPolicies) {
TopicName topicName = TopicName.get(topic);
final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
-
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
Review Comment:
This change is not necessary since the stage returned by `exceptionally()` is not being used
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1235,22 +1235,24 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
Review Comment:
Same as the other: This change is not necessary since the stage returned by exceptionally() is not being used
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1522273862
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1262,14 +1264,17 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
pulsarStats.recordTopicLoadTimeValue(topic, topicLoadLatencyMs);
addTopicToStatsMaps(TopicName.get(topic), nonPersistentTopic);
topicFuture.complete(Optional.of(nonPersistentTopic));
- }).exceptionally(ex -> {
- log.warn("Replication check failed. Removing topic from topics list {}, {}", topic, ex.getCause());
- nonPersistentTopic.stopReplProducers().whenComplete((v, exception) -> {
- pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
- topicFuture.completeExceptionally(ex);
- });
- return null;
- });
+ })
+ .exceptionally(ex -> {
+ log.warn("Replication check failed. Removing topic from topics list {}, {}",
+ topic, ex.getCause());
+ nonPersistentTopic.stopReplProducers()
+ .whenComplete((v, exception) -> {
+ pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
+ topicFuture.completeExceptionally(ex);
+ });
+ return null;
+ });
Review Comment:
OK, I got 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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun closed pull request #22256: [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed
URL: https://github.com/apache/pulsar/pull/22256
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1522282858
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1235,22 +1235,24 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
});
if (!pulsar.getConfiguration().isEnableNonPersistentTopics()) {
if (log.isDebugEnabled()) {
log.debug("Broker is unable to load non-persistent topic {}", topic);
}
- return FutureUtil.failedFuture(
+ topicFuture.completeExceptionally(
new NotAllowedException("Broker is not unable to load non-persistent topic"));
+ return topicFuture;
}
final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
NonPersistentTopic nonPersistentTopic;
try {
nonPersistentTopic = newTopic(topic, null, this, NonPersistentTopic.class);
} catch (Throwable e) {
log.warn("Failed to create topic {}", topic, e);
- return FutureUtil.failedFuture(e);
+ topicFuture.completeExceptionally(e);
+ return topicFuture;
Review Comment:
How does it make a difference?
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun closed pull request #22256: [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed
URL: https://github.com/apache/pulsar/pull/22256
--
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
Re: [PR] [fix] flaky test: BrokerServiceTest.testBrokerStatsTopicLoadFailed [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #22256:
URL: https://github.com/apache/pulsar/pull/22256#discussion_r1522269165
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1235,22 +1235,24 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
CompletableFuture<Optional<Topic>> topicFuture = new CompletableFuture<>();
topicFuture.exceptionally(t -> {
pulsarStats.recordTopicLoadFailed();
- return null;
+ return Optional.empty();
});
if (!pulsar.getConfiguration().isEnableNonPersistentTopics()) {
if (log.isDebugEnabled()) {
log.debug("Broker is unable to load non-persistent topic {}", topic);
}
- return FutureUtil.failedFuture(
+ topicFuture.completeExceptionally(
new NotAllowedException("Broker is not unable to load non-persistent topic"));
+ return topicFuture;
}
final long topicCreateTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime());
NonPersistentTopic nonPersistentTopic;
try {
nonPersistentTopic = newTopic(topic, null, this, NonPersistentTopic.class);
} catch (Throwable e) {
log.warn("Failed to create topic {}", topic, e);
- return FutureUtil.failedFuture(e);
+ topicFuture.completeExceptionally(e);
+ return topicFuture;
Review Comment:
these changes don't seem to be relevant to fixing the flaky test.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1262,14 +1264,17 @@ private CompletableFuture<Optional<Topic>> createNonPersistentTopic(String topic
pulsarStats.recordTopicLoadTimeValue(topic, topicLoadLatencyMs);
addTopicToStatsMaps(TopicName.get(topic), nonPersistentTopic);
topicFuture.complete(Optional.of(nonPersistentTopic));
- }).exceptionally(ex -> {
- log.warn("Replication check failed. Removing topic from topics list {}, {}", topic, ex.getCause());
- nonPersistentTopic.stopReplProducers().whenComplete((v, exception) -> {
- pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
- topicFuture.completeExceptionally(ex);
- });
- return null;
- });
+ })
+ .exceptionally(ex -> {
+ log.warn("Replication check failed. Removing topic from topics list {}, {}",
+ topic, ex.getCause());
+ nonPersistentTopic.stopReplProducers()
+ .whenComplete((v, exception) -> {
+ pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
+ topicFuture.completeExceptionally(ex);
+ });
+ return null;
+ });
Review Comment:
this looks like a plain formatting change. It's better to avoid such 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