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/22 06:01:18 UTC
[GitHub] [pulsar] wangjialing218 opened a new pull request #14409: Make sure policies.is_allow_auto_update_schema not null
wangjialing218 opened a new pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409
### Motivation
Follow code works fine on 2.8.x but get NPE on 2.9.x
```
Policies policies = pulsarAdmin.namespaces().getPolicies("namespacePath");
boolean allowedAutoupdateSchema = policies.is_allow_auto_update_schema;
```
Becasue `Policies.is_allow_auto_update_schema` changed from `boolean` to `Boolean` by #12786 and default value is null.
This may cause user's service fail when upgrade pulsar from 2.8.x to 2.9.x
### Modifications
Make sure `Policies.is_allow_auto_update_schema` not null when call `getPolicies`
### Documentation
- [x] `no-need-doc`
Just keep behaviour same between 2.8.x and 2.9.x
--
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 #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048719879
> We can go with the current form from my opinion.
I would like to see the 'applied' version of the function some day.
Agree, maybe we can plan to 2.11
--
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] michaeljmarshall commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048435688
@codelipenghui - thanks for tagging me. Looking 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
[GitHub] [pulsar] eolivelli commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048525313
We can go with the current form from my opinion.
I would like to see the 'applied' version of the function some day.
Let's unblock the release.
--
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 #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048720876
@eolivelli @Jason918 @michaeljmarshall Please help review again
--
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] eolivelli merged pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409
--
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] wangjialing218 commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048688516
/pulsarbot run-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 commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048434900
@gaoran10 @michaeljmarshall Please help check this PR, which is fixing a breaking change in the ongoing release 2.8.3, 2.9.2, and 2.10.0. And @wangjialing218 also shared the information under the 2.9.2 VOTE3 email thread, we need to decide as soon as possible whether to start a new round RC.
--
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 #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1047742951
> We can add a new interface Policies getPolicies(String namespace, boolean applied) in org.apache.pulsar.client.admin.Namespaces. And set applied as true by default in getPolicies(String namespace).
the applied option should be false by default? to keep consistent with topic policy does. It's better to start with a PIP, as far as I understand, not all the namespace policies follow the same way, some return null if the namespace doesn't have the policy, some return value from broker level, we need to improve this situation even if the inevitable break change may be introduced, otherwise, we will always be confused by different behaviors.
We should add a release notice for https://github.com/apache/pulsar/pull/12786
--
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 pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048430616
> the applied option should be false by default?
This default value is just for backward compatible. And we can even deprecate method `getPolicies(String namespace)`.
For user with new version of client and broker, the `applied` parameter should always be provided by user.
Just one option to keep compatibility.
--
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] wangjialing218 commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048466907
> > I believe this solution is incomplete. I think we also need to update this method defined on line 517 in the same class:
> > ```java
> > protected Policies getNamespacePolicies(String tenant, String cluster, String namespace)
> > ```
>
> After looking at the code a bit more, it appears that updating the second `getNamespacePolicies` method is unnecessary. However, it would probably be appropriate to keep things consistent.
@michaeljmarshall Done, the second method was duplicated code from first one.
--
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] wangjialing218 commented on a change in pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on a change in pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#discussion_r812576040
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -293,6 +293,9 @@ protected Policies getNamespacePolicies(NamespaceName namespaceName) {
BundlesData bundleData = pulsar().getNamespaceService().getNamespaceBundleFactory()
.getBundles(namespaceName).getBundlesData();
policies.bundles = bundleData != null ? bundleData : policies.bundles;
+ if (policies.is_allow_auto_update_schema == null) {
Review comment:
Done
--
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] michaeljmarshall commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048445848
> > the applied option should be false by default?
>
> This default value is just for backward compatible. And we can even deprecate method `getPolicies(String namespace)`. For user with new version of client and broker, the `applied` parameter should always be provided by user. Just one option to keep compatibility.
This makes sense to me. Do you think we need to add the `applied` logic to this PR @Jason918?
--
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 #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048433468
> This default value is just for backward compatible. And we can even deprecate method getPolicies(String namespace).
For user with new version of client and broker, the applied parameter should always be provided by user.
Just one option to keep compatibility.
I see, we have to make tradeoffs here, maybe we should make API follow the same pattern in 3.0
--
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] wangjialing218 commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1047530880
/pulsarbot run-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] michaeljmarshall commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048444479
> I believe this solution is incomplete. I think we also need to update this method defined on line 517 in the same class:
>
> ```java
> protected Policies getNamespacePolicies(String tenant, String cluster, String namespace)
> ```
After looking at the code a bit more, it appears that updating the second `getNamespacePolicies` method is unnecessary. However, it would probably be appropriate to keep things consistent.
--
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 #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#discussion_r812550895
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -293,6 +293,9 @@ protected Policies getNamespacePolicies(NamespaceName namespaceName) {
BundlesData bundleData = pulsar().getNamespaceService().getNamespaceBundleFactory()
.getBundles(namespaceName).getBundlesData();
policies.bundles = bundleData != null ? bundleData : policies.bundles;
+ if (policies.is_allow_auto_update_schema == null) {
Review comment:
Please add a comment here to indicate to return the broker value here is for keeping compatibility.
--
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] michaeljmarshall commented on pull request #14409: Make sure policies.is_allow_auto_update_schema not null
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14409:
URL: https://github.com/apache/pulsar/pull/14409#issuecomment-1048467366
@wangjialing218 - great, I thought those methods looked similar, but I didn't look closely enough to see the one was a duplicate. Thanks for fixing that.
--
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