eolivelli commented on code in PR #15576:
URL: https://github.com/apache/pulsar/pull/15576#discussion_r872045946
##########
pulsar-common/src/main/java/org/apache/pulsar/client/admin/internal/data/AuthPoliciesImpl.java:
##########
@@ -42,6 +42,10 @@ public final class AuthPoliciesImpl implements AuthPolicies {
@JsonProperty("subscription_auth_roles")
private Map<String, Set<String>> subscriptionAuthentication = new
TreeMap<>();
+ // Default value is set in the builder
+ @JsonProperty(value = "implicit_subscription_auth")
+ private boolean implicitSubscriptionAuth;
Review Comment:
adding a field with default value "true" is problematic, thinking to the
upgrade path.
This is because on storage (ZK) all the old Policies do not have this field,
but we are considering it "true" if it is not present.
This may make us fall into some subtle problems with clusters upgraded to
the latest version.
I suggest to go down these ways:
1) use Boolean (nullable boolean) and deal explicitly with the case "unknown"
2) negate the flag, this way the default will be "false" and this value will
also be applied in case of null/non existing field (so old cluster upgraded, or
updates happening during rolling upgrades of the cluster
I prefer 2) as it is easier to deal with both code changes and also with the
migration
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
