You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "coderzc (via GitHub)" <gi...@apache.org> on 2023/07/09 09:43:18 UTC
[GitHub] [pulsar] coderzc opened a new pull request, #20763: [fix][broker] Fix get topic policies as null during clean cache
coderzc opened a new pull request, #20763:
URL: https://github.com/apache/pulsar/pull/20763
### Motivation
Currently, we may get topic policies as null during the clean topic policies since there is a race condition between them.
| | thread1 `getTopicPolicies` | thread2 `cleanCacheAndCloseReader` |
| ---- | ------------------------------------------------------------ | ------------------------------------------------------------ |
| T1 | check if policyCacheInitMap exists the value of this namespace | remove the reader of this namespace from readerCaches |
| T2 | the value of this namespace in the policyCacheInitMap is `true` | **remove the policies of this namespace from policiesCache** |
| T3 | **gets the topicPolicies of this namespace from policiesCache as `null`** | |
| T4 | | **remove the value of this namespace from policyCacheInitMap** |
### Modifications
Use `synchronized` to make get topic policies and clean cache mutually exclusive.
### Verifying this change
- [x] 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*
### 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: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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 #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #20763:
URL: https://github.com/apache/pulsar/pull/20763
--
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] mattisonchao commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1257647573
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -75,7 +75,8 @@ public class SystemTopicBasedTopicPoliciesService implements TopicPoliciesServic
private final Map<NamespaceName, AtomicInteger> ownedBundlesCountPerNamespace = new ConcurrentHashMap<>();
- private final Map<NamespaceName, CompletableFuture<SystemTopicClient.Reader<PulsarEvent>>>
+ @VisibleForTesting
Review Comment:
Why public this field?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -75,7 +75,8 @@ public class SystemTopicBasedTopicPoliciesService implements TopicPoliciesServic
private final Map<NamespaceName, AtomicInteger> ownedBundlesCountPerNamespace = new ConcurrentHashMap<>();
- private final Map<NamespaceName, CompletableFuture<SystemTopicClient.Reader<PulsarEvent>>>
+ @VisibleForTesting
Review Comment:
Why make this field to be public?
--
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] coderzc commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1260760193
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java:
##########
@@ -3015,6 +3019,63 @@ public void testLoopCreateAndDeleteTopicPolicies() throws Exception {
});
}
}
+
+ @Test
+ public void testGetTopicPoliciesWithCleanCache()
Review Comment:
@codelipenghui PTAL.
--
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] mattisonchao commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1257648404
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -220,12 +221,13 @@ public TopicPolicies getTopicPolicies(TopicName topicName,
NamespaceName namespace = topicName.getNamespaceObject();
prepareInitPoliciesCache(namespace, new CompletableFuture<>());
}
- if (policyCacheInitMap.containsKey(topicName.getNamespaceObject())
- && !policyCacheInitMap.get(topicName.getNamespaceObject())) {
- throw new TopicPoliciesCacheNotInitException();
+ synchronized (this) {
Review Comment:
Can we consider relying on `policyCacheInitMap` to avoid `synchronized`?
--
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] mattisonchao commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1260549982
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java:
##########
@@ -3015,6 +3015,7 @@ public void testLoopCreateAndDeleteTopicPolicies() throws Exception {
});
}
}
+
Review Comment:
can you delete the blank line?
--
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] coderzc commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1257700516
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -75,7 +75,8 @@ public class SystemTopicBasedTopicPoliciesService implements TopicPoliciesServic
private final Map<NamespaceName, AtomicInteger> ownedBundlesCountPerNamespace = new ConcurrentHashMap<>();
- private final Map<NamespaceName, CompletableFuture<SystemTopicClient.Reader<PulsarEvent>>>
+ @VisibleForTesting
Review Comment:
Fixed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -220,12 +221,13 @@ public TopicPolicies getTopicPolicies(TopicName topicName,
NamespaceName namespace = topicName.getNamespaceObject();
prepareInitPoliciesCache(namespace, new CompletableFuture<>());
}
- if (policyCacheInitMap.containsKey(topicName.getNamespaceObject())
- && !policyCacheInitMap.get(topicName.getNamespaceObject())) {
- throw new TopicPoliciesCacheNotInitException();
+ synchronized (this) {
Review Comment:
Good idea.
--
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 diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1257992991
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java:
##########
@@ -3015,6 +3019,63 @@ public void testLoopCreateAndDeleteTopicPolicies() throws Exception {
});
}
}
+
+ @Test
+ public void testGetTopicPoliciesWithCleanCache()
Review Comment:
@coderzc Can you reproduce the issue with this test with the fix? I have tried it on my laptop without this fix. But can't reproduce the issue. The fix looks good, just the test can't protect the changes of 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] coderzc closed pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc closed pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
URL: https://github.com/apache/pulsar/pull/20763
--
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] codecov-commenter commented on pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#issuecomment-1632264766
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20763?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20763](https://app.codecov.io/gh/apache/pulsar/pull/20763?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (eae2a70) into [master](https://app.codecov.io/gh/apache/pulsar/commit/0bac873cba1b39f888fe359368217e8ac030f29a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0bac873) will **increase** coverage by `36.31%`.
> The diff coverage is `92.85%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20763?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20763 +/- ##
=============================================
+ Coverage 36.80% 73.11% +36.31%
- Complexity 12073 32128 +20055
=============================================
Files 1689 1866 +177
Lines 129387 139041 +9654
Branches 14120 15294 +1174
=============================================
+ Hits 47619 101661 +54042
+ Misses 75482 29322 -46160
- Partials 6286 8058 +1772
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.17% <42.85%> (+0.08%)` | :arrow_up: |
| systests | `25.01% <21.42%> (-0.11%)` | :arrow_down: |
| unittests | `72.41% <92.85%> (+40.49%)` | :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.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20763?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../service/SystemTopicBasedTopicPoliciesService.java](https://app.codecov.io/gh/apache/pulsar/pull/20763?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljQmFzZWRUb3BpY1BvbGljaWVzU2VydmljZS5qYXZh) | `76.96% <92.85%> (+18.93%)` | :arrow_up: |
... and [1431 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20763/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] coderzc commented on a diff in pull request #20763: [fix][broker] Fix get topic policies as null during clean cache
Posted by "coderzc (via GitHub)" <gi...@apache.org>.
coderzc commented on code in PR #20763:
URL: https://github.com/apache/pulsar/pull/20763#discussion_r1258209148
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java:
##########
@@ -3015,6 +3019,63 @@ public void testLoopCreateAndDeleteTopicPolicies() throws Exception {
});
}
}
+
+ @Test
+ public void testGetTopicPoliciesWithCleanCache()
Review Comment:
I have rewritten the test to make it can reproduce the issue.
--
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