codelipenghui commented on code in PR #25461:
URL: https://github.com/apache/pulsar/pull/25461#discussion_r3089854693
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2748,7 +2750,13 @@ protected void
handleGetOrCreateSchema(CommandGetOrCreateSchema commandGetOrCrea
service.getTopicIfExists(topicName).thenAccept(topicOpt -> {
if (topicOpt.isPresent()) {
Topic topic = topicOpt.get();
- CompletableFuture<SchemaVersion> schemaVersionFuture =
tryAddSchema(topic, schema);
+ boolean isReplicatorProducer = false;
+ if (commandGetOrCreateSchema.hasProducerName()) {
+ isReplicatorProducer =
Producer.isRemoteOrShadow(commandGetOrCreateSchema.getProducerName(),
+
getBrokerService().getPulsar().getConfig().getReplicatorPrefix());
+ }
+ CompletableFuture<SchemaVersion> schemaVersionFuture =
+ tryAddSchema(topic, schema, isReplicatorProducer);
Review Comment:
Will it be a security issue that user can just set producer name to
pulsar.repl.* to get rid of the schema upload control? if user disabled
is_allow_auto_update_schema
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2163,14 +2163,20 @@ private class SetIsAllowAutoUpdateSchema extends
CliCommand {
@Option(names = { "--disable", "-d" }, description = "Disable schema
validation enforced")
private boolean disable = false;
+ @Option(names = { "--disable-for-replicator" },
+ description = "By default, brokers always allow replicator to
register new compatible schemas even"
+ + " when auto updates are disabled, if you want to disable
replicators to register compatible schemas,"
+ + " please set it to true")
+ private boolean disableForReplicator;
+
Review Comment:
--disable-for-replicator is a primitive boolean defaulting to false, so
every set-is-allow-auto-update-schema call sends
allowAutoUpdateSchemaWithReplicator=true unless the flag is supplied. A user
who ran ... --disable --disable-for-replicator and later runs ... --enable
silently re-enables replicator updates — different from the REST API's
null-preserving semantics.
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2163,14 +2163,20 @@ private class SetIsAllowAutoUpdateSchema extends
CliCommand {
@Option(names = { "--disable", "-d" }, description = "Disable schema
validation enforced")
private boolean disable = false;
+ @Option(names = { "--disable-for-replicator" },
+ description = "By default, brokers always allow replicator to
register new compatible schemas even"
+ + " when auto updates are disabled, if you want to disable
replicators to register compatible schemas,"
+ + " please set it to true")
+ private boolean disableForReplicator;
Review Comment:
PIP says CLI flag is --enable-for-replicator, implementation uses
--disable-for-replicator.
##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/Policies.java:
##########
@@ -218,6 +222,8 @@ public boolean equals(Object obj) {
&& schema_validation_enforced ==
other.schema_validation_enforced
&& schema_compatibility_strategy ==
other.schema_compatibility_strategy
&& is_allow_auto_update_schema ==
other.is_allow_auto_update_schema
+ && is_allow_auto_update_schema_with_replicator
+ == other.is_allow_auto_update_schema_with_replicator
Review Comment:
Should use Objects.equals()?
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2163,14 +2163,20 @@ private class SetIsAllowAutoUpdateSchema extends
CliCommand {
@Option(names = { "--disable", "-d" }, description = "Disable schema
validation enforced")
private boolean disable = false;
+ @Option(names = { "--disable-for-replicator" },
+ description = "By default, brokers always allow replicator to
register new compatible schemas even"
+ + " when auto updates are disabled, if you want to disable
replicators to register compatible schemas,"
+ + " please set it to true")
+ private boolean disableForReplicator;
+
Review Comment:
Please also check the Java API
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -2571,13 +2571,18 @@ protected void
internalSetSchemaValidationEnforced(boolean schemaValidationEnfor
"schemaValidationEnforced");
}
- protected void internalSetIsAllowAutoUpdateSchema(boolean
isAllowAutoUpdateSchema) {
+ protected void internalSetIsAllowAutoUpdateSchema(boolean
isAllowAutoUpdateSchema,
+ Boolean
isAllowAutoUpdateSchemaWithReplicator) {
validateNamespacePolicyOperation(namespaceName,
PolicyName.SCHEMA_COMPATIBILITY_STRATEGY,
PolicyOperation.WRITE);
validatePoliciesReadOnlyAccess();
mutatePolicy((policies) -> {
policies.is_allow_auto_update_schema =
isAllowAutoUpdateSchema;
+ if (isAllowAutoUpdateSchemaWithReplicator != null) {
+ policies.is_allow_auto_update_schema_with_replicator =
+ isAllowAutoUpdateSchemaWithReplicator;
+ }
return policies;
}, (policies) -> policies.is_allow_auto_update_schema,
"isAllowAutoUpdateSchema");
Review Comment:
Missed isAllowAutoUpdateSchemaWithReplicator?
--
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]
