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 2021/11/13 13:07:55 UTC
[GitHub] [pulsar] Jason918 opened a new pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Jason918 opened a new pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
Fixes #12757
### Motivation
See #12757
### Modifications
Add a broker config : isAllowAutoUpdateSchema
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- testBrokerAllowAutoUpdateSchemaDisabled
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
**- The schema: (yes)**
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `doc`
Added doc in the annotation of new config.
--
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 a change in pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#discussion_r749048458
##########
File path: conf/broker.conf
##########
@@ -270,6 +270,10 @@ brokerMaxConnections=0
# The maximum number of connections per IP. If it exceeds, new connections are rejected.
brokerMaxConnectionsPerIp=0
+# Allow schema to be auto updated. If this is disabled, 'is_allow_auto_update_schema' in namespace
Review comment:
Added. PTAL.
--
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 #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#discussion_r751997217
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -573,6 +573,14 @@
)
private int brokerMaxConnectionsPerIp = 0;
+ @FieldContext(
+ category = CATEGORY_POLICIES,
+ dynamic = true,
+ doc = "Allow schema to be auto updated. If this is disabled, 'is_allow_auto_update_schema' in namespace "
+ + "policy will be ignored. This is enabled by default."
+ )
+ private boolean allowAutoUpdateSchema = true;
Review comment:
```suggestion
private boolean isAllowAutoUpdateSchemaEnabled = true;
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -333,20 +333,21 @@ public String getReplicatorPrefix() {
String base = TopicName.get(getName()).getPartitionedTopicName();
String id = TopicName.get(base).getSchemaName();
SchemaRegistryService schemaRegistryService = brokerService.pulsar().getSchemaRegistryService();
- return isAllowAutoUpdateSchema ? schemaRegistryService
- .putSchemaIfAbsent(id, schema, schemaCompatibilityStrategy)
- : schemaRegistryService.trimDeletedSchemaAndGetList(id).thenCompose(schemaAndMetadataList ->
- schemaRegistryService.getSchemaVersionBySchemaData(schemaAndMetadataList, schema)
- .thenCompose(schemaVersion -> {
- if (schemaVersion == null) {
- return FutureUtil
- .failedFuture(
- new IncompatibleSchemaException(
- "Schema not found and schema auto updating is disabled."));
- } else {
- return CompletableFuture.completedFuture(schemaVersion);
- }
- }));
+
+ if (brokerService.pulsar().getConfig().isAllowAutoUpdateSchema() && isAllowAutoUpdateSchema) {
Review comment:
I think it's not correct here, the namespace level policy can overwrite the broker level configuration, which means you can disable it at the broker level by default but for some namespaces, you can enable it.
It's not like a broker level mandatory control. Just the broker level default configurations.
--
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 #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968075522
/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] Jason918 commented on pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-972785430
/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] Anonymitaet commented on a change in pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#discussion_r748978824
##########
File path: conf/broker.conf
##########
@@ -270,6 +270,10 @@ brokerMaxConnections=0
# The maximum number of connections per IP. If it exceeds, new connections are rejected.
brokerMaxConnectionsPerIp=0
+# Allow schema to be auto updated. If this is disabled, 'is_allow_auto_update_schema' in namespace
Review comment:
Do we need to add docs to https://pulsar.apache.org/docs/en/next/reference-configuration/#broker and https://pulsar.apache.org/docs/en/next/reference-configuration/#standalone?
--
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 #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968639925
/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] Jason918 commented on pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968188816
> I think `allowAutoUpdateSchema` is better. the boolean type of field have implied the meaning of `is`
Nice catch.
And it's consistent with previous 'allowXXX' fields.
@Shoothzj Updated, 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] Jason918 commented on pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968282373
/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] Jason918 commented on pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968065973
@codelipenghui PTAL
--
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 #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#issuecomment-968244820
> Should we also add this config to `conf/broker.conf`, `conf/standalone.conf` and releated `document`?
Thanks for the reminding, added the config to these files. Please take a look.
About the user doc, the config doc should be auto-generated by the annotation, right ?
--
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 merged pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #12786:
URL: 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] codelipenghui commented on a change in pull request #12786: [Issue 12757][broker] add broker config isAllowAutoUpdateSchema
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12786:
URL: https://github.com/apache/pulsar/pull/12786#discussion_r752033446
##########
File path: conf/broker.conf
##########
@@ -270,6 +270,10 @@ brokerMaxConnections=0
# The maximum number of connections per IP. If it exceeds, new connections are rejected.
brokerMaxConnectionsPerIp=0
+# Allow schema to be auto updated. If this is disabled, 'is_allow_auto_update_schema' in namespace
Review comment:
@Jason918 I think the description of the configuration is not correct?
--
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