Make sure policies.is_allow_auto_update_schema not null

[wangjialing218]

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

• no-need-doc

  Just keep behaviour same between 2.8.x and 2.9.x

[wangjialing218]

/pulsarbot run-failure-checks

[eolivelli]

We are used to allow getting the configuration merged with the global configuration using 'applied' parameter.

I can't give pointers now, but you can learn learn 'applied' in other APIs

[Jason918]

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).

In broker, add a new parameter @QueryParam("applied") @DefaultValue("true") boolean applied in org.apache.pulsar.broker.admin.v2.Namespaces#getPolicies.

NOTE: you may need to update other policies filed to applied value too.

[codelipenghui]

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 #12786

[Jason918]

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.

[codelipenghui]

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

[codelipenghui]

@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.

[michaeljmarshall]

@codelipenghui - thanks for tagging me. Looking now.

[codelipenghui]

The change looks good to me, just left a minor comment.

[michaeljmarshall]

I believe this solution is incomplete. I think we also need to update this method defined on line 517 in the same class:

protected Policies getNamespacePolicies(String tenant, String cluster, String namespace)

[michaeljmarshall]

I believe this solution is incomplete. I think we also need to update this method defined on line 517 in the same class:

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]

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?

[wangjialing218]

I believe this solution is incomplete. I think we also need to update this method defined on line 517 in the same class:

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.

[michaeljmarshall]

@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.

[michaeljmarshall]

I don't have a strong opinion regarding the applied flag. I think we could probably add it to this endpoint without breaking the API for 2.10. I am interested to know what @eolivelli has to say.

I am +1 on the PR when ignoring the question of the applied flag.

[eolivelli]

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.

[Jason918]

LGTM

[wangjialing218]

/pulsarbot run-failure-checks

[codelipenghui]

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

[codelipenghui]

@eolivelli @Jason918 @michaeljmarshall Please help review again

[eolivelli]

+1