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/03/01 08:31:04 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request #14503: [Broker] Fixed erroneous behavior caused by not cleaning up topic policy service state.
mattisonchao opened a new pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503
### Motivation
The current exception handler does not clean all states, which will cause some wrong behaviour.
**E.g:**
case 1:
https://github.com/apache/pulsar/blob/e20f34bd2a6b4bd7ce72390a593931a855f8e250/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L249-L264
when line 259 closes the reader by exception, it does not remove ``policyCacheInitMap ``, so when next invoke this method they will do nothing because ``policyCacheInitMap.putIfAbsent(namespace, false)`` is not null.
then line 201 will always throw.
https://github.com/apache/pulsar/blob/e20f34bd2a6b4bd7ce72390a593931a855f8e250/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L193-L205
case 2:
When line 384 gets "AlreadyClosedException", it doesn't delete "policyCacheInitMap", but the current namespace value in "policyCacheInitMap" is true. Therefore, the next time ``prepareInitPoliciesCache `` is called, a new reader is never created. then we will always get a null value.
https://github.com/apache/pulsar/blob/e20f34bd2a6b4bd7ce72390a593931a855f8e250/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L376-L393
### Modifications
- Clean state when the reader gets an error.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
--
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 change in pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816892656
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
Review comment:
@codelipenghui @Technoboy-
Threading issues will be caused by the point in time when reader failures and new reader creations occur at the same time.
We create a new Reader using the "prepareInitPoliciesCache" method, which uses the "ConcurrentHashMap" to ensure that the "policyCacheInitMap" is atomic.
Therefore, we can clean this atomic value last when cleaning to ensure that cleanup and new creation are atomic.
I'm not sure if this is a thread safety issue as you think. Please let me know if I understand wrong.
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056527286
/pulsarbot rerun-failure-checks
--
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- edited a comment on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- edited a comment on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1061409837
>
As @mattisonchao is on holiday this week, how about merging this first and adding test later?
@lhotari @Jason918 @codelipenghui
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1061409837
>
As @mattisonchao is on holiday this week, how about merging this first and add test later?
@lhotari @Jason918 @codelipenghui
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056024937
/pulsarbot rerun-failure-checks
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816878207
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
Review comment:
Move `policyCacheInitMap.remove(namespace)` to the end of this method to ensure the consistent state.
@[mattisonchao](https://github.com/mattisonchao)
--
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 closed pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056059729
/pulsarbot rerun-failure-checks
--
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 removed a comment on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056059729
/pulsarbot rerun-failure-checks
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1055986345
/pulsarbot rerun-failure-checks
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816735623
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
+ CompletableFuture<SystemTopicClient.Reader<PulsarEvent>> readerFuture = readerCaches.remove(namespace);
+ policyCacheInitMap.remove(namespace);
+ policiesCache.entrySet().removeIf(entry -> entry.getKey().getNamespaceObject() == namespace);
+ if (cleanOwnedBundlesCount) {
+ ownedBundlesCountPerNamespace.remove(namespace);
+ }
+ if (readerFuture != null) {
Review comment:
Should check the `readerFuture` does not complete with exception.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -382,10 +382,10 @@ private void readMorePolicies(SystemTopicClient.Reader<PulsarEvent> reader) {
} else {
if (ex instanceof PulsarClientException.AlreadyClosedException) {
log.error("Read more topic policies exception, close the read now!", ex);
- NamespaceName namespace = reader.getSystemTopic().getTopicName().getNamespaceObject();
- ownedBundlesCountPerNamespace.remove(namespace);
- readerCaches.remove(namespace);
+ cleanCacheAndCloseReader(
+ reader.getSystemTopic().getTopicName().getNamespaceObject(), false);
} else {
+ log.warn("Read more topic polices exception, read again. {}", ex.getMessage());
Review comment:
Yes, it's better to have the stacktrace.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
Review comment:
Do we need to consider the thread-safe issue here?
--
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 removed a comment on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1055986345
/pulsarbot rerun-failure-checks
--
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 removed a comment on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056024937
/pulsarbot rerun-failure-checks
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816564409
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -382,10 +382,10 @@ private void readMorePolicies(SystemTopicClient.Reader<PulsarEvent> reader) {
} else {
if (ex instanceof PulsarClientException.AlreadyClosedException) {
log.error("Read more topic policies exception, close the read now!", ex);
- NamespaceName namespace = reader.getSystemTopic().getTopicName().getNamespaceObject();
- ownedBundlesCountPerNamespace.remove(namespace);
- readerCaches.remove(namespace);
+ cleanCacheAndCloseReader(
+ reader.getSystemTopic().getTopicName().getNamespaceObject(), false);
} else {
+ log.warn("Read more topic polices exception, read again. {}", ex.getMessage());
Review comment:
Stacktrace is better? I'm not sure.
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1058941908
Good work @mattisonchao
I guess it might be hard to reproduce with tests, but it would be useful to have a test case that reproduces the issue.
Penghui's comment about the thread safety (race condition) is valid. that might require more changes. Tests would help in that case too.
--
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 change in pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816881811
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -382,10 +382,10 @@ private void readMorePolicies(SystemTopicClient.Reader<PulsarEvent> reader) {
} else {
if (ex instanceof PulsarClientException.AlreadyClosedException) {
log.error("Read more topic policies exception, close the read now!", ex);
- NamespaceName namespace = reader.getSystemTopic().getTopicName().getNamespaceObject();
- ownedBundlesCountPerNamespace.remove(namespace);
- readerCaches.remove(namespace);
+ cleanCacheAndCloseReader(
+ reader.getSystemTopic().getTopicName().getNamespaceObject(), false);
} else {
+ log.warn("Read more topic polices exception, read again. {}", ex.getMessage());
Review comment:
Fixed
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
+ CompletableFuture<SystemTopicClient.Reader<PulsarEvent>> readerFuture = readerCaches.remove(namespace);
+ policyCacheInitMap.remove(namespace);
+ policiesCache.entrySet().removeIf(entry -> entry.getKey().getNamespaceObject() == namespace);
+ if (cleanOwnedBundlesCount) {
+ ownedBundlesCountPerNamespace.remove(namespace);
+ }
+ if (readerFuture != null) {
Review comment:
Fixed
--
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 change in pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816892656
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
Review comment:
@codelipenghui
Threading issues will be caused by the point in time when reader failures and new reader creations occur at the same time.
We create a new Reader using the "prepareInitPoliciesCache" method, which uses the "ConcurrentHashMap" to ensure that the "policyCacheInitMap" is atomic.
Therefore, we can clean this atomic value last when cleaning to ensure that cleanup and new creation are atomic.
I'm not sure if this is a thread safety issue as you think. Please let me know if I understand wrong.
--
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 removed a comment on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056112143
/pulsarbot rerun-failure-checks
--
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 change in pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r819345062
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
+ CompletableFuture<SystemTopicClient.Reader<PulsarEvent>> readerFuture = readerCaches.remove(namespace);
+ policiesCache.entrySet().removeIf(entry -> entry.getKey().getNamespaceObject() == namespace);
Review comment:
Fixed, 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] codelipenghui commented on pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1061411129
> As @mattisonchao is on holiday this week, how about merging this first and adding test later?
Works for me, @JipeiWang could you please help create an issue for tracking the test?
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1056112143
/pulsarbot rerun-failure-checks
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1059862026
> Good work @mattisonchao I guess it might be hard to reproduce with tests, but it would be useful to have a test case that reproduces the issue. Penghui's comment about the thread safety (race condition) is valid. that might require more changes. Tests would help in that case too.
Sure, Very thanks for your suggestions.
I will try to add a multi-thread test to verify race conditions.
--
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 pull request #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#issuecomment-1057754585
@codelipenghui @Technoboy- @eolivelli @lhotari @michaeljmarshall @Jason918
PTAL, when you have time ~
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r819248497
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
+ CompletableFuture<SystemTopicClient.Reader<PulsarEvent>> readerFuture = readerCaches.remove(namespace);
+ policiesCache.entrySet().removeIf(entry -> entry.getKey().getNamespaceObject() == namespace);
Review comment:
`equals` instead of `==` ?
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816564409
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -382,10 +382,10 @@ private void readMorePolicies(SystemTopicClient.Reader<PulsarEvent> reader) {
} else {
if (ex instanceof PulsarClientException.AlreadyClosedException) {
log.error("Read more topic policies exception, close the read now!", ex);
- NamespaceName namespace = reader.getSystemTopic().getTopicName().getNamespaceObject();
- ownedBundlesCountPerNamespace.remove(namespace);
- readerCaches.remove(namespace);
+ cleanCacheAndCloseReader(
+ reader.getSystemTopic().getTopicName().getNamespaceObject(), false);
} else {
+ log.warn("Read more topic polices exception, read again. {}", ex.getMessage());
Review comment:
Stack or message? I' not sure.
--
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 #14503: [Broker] Fixed wrong behaviour caused by not cleaning up topic policy service state.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14503:
URL: https://github.com/apache/pulsar/pull/14503#discussion_r816878207
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -373,6 +361,18 @@ private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> reader, Comp
});
}
+ private void cleanCacheAndCloseReader(NamespaceName namespace, boolean cleanOwnedBundlesCount) {
Review comment:
Move `policyCacheInitMap.remove(namespace)` to the end of this method to ensure the state.
--
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