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/08/09 12:36:37 UTC
[GitHub] [pulsar] wuzhanpeng opened a new pull request #11606: Fix race condition in concurrent schema deletion
wuzhanpeng opened a new pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606
This PR may fixed #11605
### Motivation
Concurrently deleting topics with the same schema may cause race condition in broker side. If we do not handle these scenarios correctly we will get unexpected exceptions in broker logs.
### Modifications
1. Add existence checks before schema deletion in `AbstractTopic#deleteSchema`.
2. Add existence checks before actually performing schema storage deletion in `BookkeeperSchemaStorage#deleteSchema`.
3. Ignore `NoNodeException` in `BookkeeperSchemaStorage#deleteSchema`.
### 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: no
- 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
This PR does not need to update the docs.
--
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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-899366052
> LGTM! could you please add some tests for it?
Sorry for the late. I supplemented a test case. PTAL~ @congbobo184
--
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 a change in pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#discussion_r685710425
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -372,6 +374,21 @@ public String getReplicatorPrefix() {
String id = TopicName.get(base).getSchemaName();
SchemaRegistryService schemaRegistryService = brokerService.pulsar().getSchemaRegistryService();
return schemaRegistryService.getSchema(id)
+ .exceptionally(t -> {
+ if (t.getCause() != null
Review comment:
This condition looks a little bit hacky.
1) we are only checking `t.getCause()`, IMHO we should have some utility method that traverses the full exception chain, looking for a SchemaException
2) a "non recoverable" SchemaException is not enough to say that this is a "Schema does not exist" exception, can we introduce some specific SchemaException subclass or method like `isRecoverable()` ?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java
##########
@@ -370,7 +377,21 @@ public void close() throws Exception {
@NotNull
private CompletableFuture<Long> deleteSchema(String schemaId, boolean forceFully) {
- return (forceFully ? CompletableFuture.completedFuture(null) : getSchema(schemaId))
+ return (forceFully ? CompletableFuture.completedFuture(null) : getSchema(schemaId).exceptionally(t -> {
+ if (t.getCause() != null
+ && (t.getCause() instanceof SchemaException)
Review comment:
same as above, at least we should have one utility method to detect this case
--
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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-896450094
/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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-895704819
/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] wuzhanpeng commented on a change in pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on a change in pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#discussion_r685787550
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -372,6 +374,21 @@ public String getReplicatorPrefix() {
String id = TopicName.get(base).getSchemaName();
SchemaRegistryService schemaRegistryService = brokerService.pulsar().getSchemaRegistryService();
return schemaRegistryService.getSchema(id)
+ .exceptionally(t -> {
+ if (t.getCause() != null
Review comment:
Thank you for your reply!
For question 1, the condition is inspired by `SchemaRegistryServiceImpl#trimDeletedSchemaAndGetList(java.lang.String)`, that is, those schemas with unrecoverable exceptions would be regarded as deleted schemas. Therefore here I continue to use the design concept.
For question 2, I agree that unrecoverable SchemaException is not equivant to "Schema does not exist". However in schema storage deletion scenario, an unrecoverable exception, which includes `NoSuchLedgerException` and `NoSuchEntryException`, is enough to say that the schema has already been deleted by others and we can quit the process.
--
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] sijie merged pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606
--
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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-899404068
/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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-899438017
/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] eolivelli commented on a change in pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#discussion_r685804787
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java
##########
@@ -372,6 +374,21 @@ public String getReplicatorPrefix() {
String id = TopicName.get(base).getSchemaName();
SchemaRegistryService schemaRegistryService = brokerService.pulsar().getSchemaRegistryService();
return schemaRegistryService.getSchema(id)
+ .exceptionally(t -> {
+ if (t.getCause() != null
Review comment:
Thanks for your clarification
What about creating an utility method and reduce code duplication?
--
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] wuzhanpeng commented on pull request #11606: Fix race condition in concurrent schema deletion
Posted by GitBox <gi...@apache.org>.
wuzhanpeng commented on pull request #11606:
URL: https://github.com/apache/pulsar/pull/11606#issuecomment-895666932
/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