You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Technoboy- (via GitHub)" <gi...@apache.org> on 2023/03/21 08:07:02 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request, #19875: [fix][broker] Fix NPE when update topic policy.
Technoboy- opened a new pull request, #19875:
URL: https://github.com/apache/pulsar/pull/19875
### Motivation
```
Failed to create consumer: consumerId=30, Cannot invoke "java.util.Map.get(Object)" because "this.subscriptionPolicies" is null
```
### 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 -->
--
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 diff in pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143340448
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
Ah, yes, right. `subscriptionPolicies` could be null.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
`subscriptionPolicies` shouldn't be null, only `updateTopicPolicy ` may change it, so I think it's better to modify line-243.
--
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 diff in pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143245493
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
`subscriptionPolicies` shouldn't be null, only `updateTopicPolicy ` may change it, so I think it's better to modify line-243.
--
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 #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143406641
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
I am wondering why `subscriptionPolicies` can be null. Is it passed by `TopicPoliceis`? Because I saw many places invoke `TopicPoliceis#getSubscriptionPolicies.get(xxx)` without an NPE check.
Should we consider avoiding this value as null?
Please correct me if I got something wrong.
/cc @Technoboy- @poorbarcode
--
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] BewareMyPower commented on pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#issuecomment-1537034984
Revert this PR from branch-2.10 because Pulsar 2.10.x does not have the `SubscriptionPolicies`.
--
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 #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143406641
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
I am wondering why `subscriptionPolicies` can be null. Is it passed by `TopicPoliceis`? Because I saw many places invoke `TopicPoliceis#getSubscriptionPolicies.get(xxx)` without an NPE check.
Should we consider avoiding this value as null?
Please correct me if I got something wrong.
/cc @Technoboy- @poorbarcode
--
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 diff in pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143554558
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
> I am wondering why subscriptionPolicies can be null
Maybe for old datas ? I‘m not sure either.
--
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- closed pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19875: [fix][broker] Fix NPE when update topic policy.
URL: https://github.com/apache/pulsar/pull/19875
--
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] poorbarcode commented on a diff in pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19875:
URL: https://github.com/apache/pulsar/pull/19875#discussion_r1143067621
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -240,7 +240,9 @@ protected void updateTopicPolicy(TopicPolicies data) {
topicPolicies.getDispatchRate().updateTopicValue(DispatchRateImpl.normalize(data.getDispatchRate()));
topicPolicies.getSchemaValidationEnforced().updateTopicValue(data.getSchemaValidationEnforced());
topicPolicies.getEntryFilters().updateTopicValue(data.getEntryFilters());
- this.subscriptionPolicies = data.getSubscriptionPolicies();
+ if (data.getSubscriptionPolicies() != null) {
Review Comment:
Should we check line-174 instead of this line?
https://github.com/apache/pulsar/blob/1e44ba179bfc189d22dff046a1887e12842b0851/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java#L173-L178
--
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- merged pull request #19875: [fix][broker] Fix NPE when update topic policy.
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #19875:
URL: https://github.com/apache/pulsar/pull/19875
--
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