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 2022/03/11 04:42:19 UTC
[GitHub] [pulsar] aloyszhang opened a new pull request #14655: Support delete schema forcefully
aloyszhang opened a new pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655
Fixes #14654
### Motivation
Currently, pulsar admin command for schemas can't delete a schema completely, it only appends a deleted `schemaInfo` to mark this schema as deleted.
This may lead to some problems as described in #14654
This pull request aims to delete schema forcefully which will remove the schema from the metastore and bookkeeper completely.
### Modifications
1. Add a new option for `DeleteSchema` to indicate delete schema forcefully or not, default is false
2. If delete schema forcefully, remove it from metastore and bookkeeper
3. Add test for force delete schema in `SchemaDeleteTest.schemaForceDeleteTest`
### Verifying this change
### 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): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
Check the box below or label this PR directly (if you have committer privilege).
- [ ] `no-need-doc`
--
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 #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r826497736
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Schemas.java
##########
@@ -98,6 +98,22 @@
*/
CompletableFuture<Void> deleteSchemaAsync(String topic);
+ /**
+ * Delete the schema associated with a given <tt>topic</tt>.
+ *
+ * @param topic topic name, in fully qualified format
+ * @param force force to delete schema
Review comment:
```suggestion
* @param force to delete schema
```
And it's better to provide more information about what is the difference after use force==true, it looks like if force==false, which means it's a mark deletion operation, will not remove any resources, but with force==true, it will remove all the resources used by schema.
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
+ "--force" }, description = "Delete schema forcefully")
Review comment:
Same as the above comment, it's better to provide more information to understand what is the difference after use `--force`
--
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] aloyszhang commented on pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#issuecomment-1066878844
@yuruguo 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] aloyszhang commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825545252
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistry.java
##########
@@ -37,7 +37,7 @@
CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId, SchemaData schema,
SchemaCompatibilityStrategy strategy);
- CompletableFuture<SchemaVersion> deleteSchema(String schemaId, String user);
+ CompletableFuture<SchemaVersion> deleteSchema(String schemaId, String user, boolean force);
Review comment:
It's an alternative way. Since this method is not oriented to users(not a breaking change), IMO, we can modify the original method instead of adding a new one. Adding a new method will leave more duplicated codes, I did see many benefits for it.
--
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] aloyszhang commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r826532004
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Schemas.java
##########
@@ -98,6 +98,22 @@
*/
CompletableFuture<Void> deleteSchemaAsync(String topic);
+ /**
+ * Delete the schema associated with a given <tt>topic</tt>.
+ *
+ * @param topic topic name, in fully qualified format
+ * @param force force to delete schema
Review comment:
ok, will add some descriptions
--
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 #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825252404
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
Review comment:
From a user's perspective, when should they delete schema forcefully?
Can we delete all schema forcefully?
--
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] aloyszhang merged pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang merged pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655
--
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] yuruguo commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
yuruguo commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825520065
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistry.java
##########
@@ -37,7 +37,7 @@
CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId, SchemaData schema,
SchemaCompatibilityStrategy strategy);
- CompletableFuture<SchemaVersion> deleteSchema(String schemaId, String user);
+ CompletableFuture<SchemaVersion> deleteSchema(String schemaId, String user, boolean force);
Review comment:
Should we remain method `deleteSchema(String schemaId, String user)` then add new method `deleteSchema(String schemaId, String user, boolean force)`?
--
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] aloyszhang commented on pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#issuecomment-1067595236
/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] aloyszhang commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825290898
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
Review comment:
> From a user's perspective, when should they delete schema forcefully?
A case I have met is all replicas of the ledger corresponding to the schema of the health-check topic were deleted (during the offline process of bookies), resulting in the failure of health-check and the failure is unrecoverable(can't update or delete the schema). I think the common topics also have the same problem.
> Can we delete all schema forcefully?
Do you mean all schemas for all topics? No, we can't
--
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] aloyszhang commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825388164
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
Review comment:
> I mean, is it safe to always use "delete -f" instead of normal delete?
It's not safe here.
If we delete a schema like before, there will leave a new ledger for the schema. But with `-f`, all ledgers will be removed.
One place the ledgers of the schema shown is `internal-stats`, with or without `-f`, we will get a different result.
> Or delete forcefully should only be used to recover unexpected state (like your case, schema data deleted by offline process).
Delete forcefully should be used if we want to delete schema completely or to recover an unexpected state
--
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 #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825376629
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
Review comment:
> > Can we delete all schema forcefully?
>
> Do you mean all schemas for all topics? No, we can't
I mean, is it safe to always use "delete -f" instead of normal delete? Or delete forcefully should only be used to recover unexpected state (like your case, schema data deleted by offline 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] github-actions[bot] commented on pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#issuecomment-1064766922
@aloyszhang:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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] github-actions[bot] commented on pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#issuecomment-1064772516
@aloyszhang:Thanks for providing doc info!
--
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 #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825376934
##########
File path: pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/SchemasImpl.java
##########
@@ -127,15 +127,25 @@ public void failed(Throwable throwable) {
@Override
public void deleteSchema(String topic) throws PulsarAdminException {
- sync(() ->deleteSchemaAsync(topic));
+ deleteSchema(topic, false);
}
@Override
public CompletableFuture<Void> deleteSchemaAsync(String topic) {
- TopicName tn = TopicName.get(topic);
+ return deleteSchemaAsync(topic, false);
+ }
+
+ @Override
+ public void deleteSchema(String topic, boolean force) throws PulsarAdminException {
+ sync(() ->deleteSchemaAsync(topic, force));
Review comment:
```suggestion
sync(() -> deleteSchemaAsync(topic, force));
```
--
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] aloyszhang commented on a change in pull request #14655: Support delete schema forcefully
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #14655:
URL: https://github.com/apache/pulsar/pull/14655#discussion_r825388164
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSchemas.java
##########
@@ -78,10 +78,14 @@ void run() throws Exception {
@Parameter(description = "persistent://tenant/namespace/topic", required = true)
private java.util.List<String> params;
+ @Parameter(names = { "-f",
Review comment:
> I mean, is it safe to always use "delete -f" instead of normal delete?
It's not safe here.
If we delete a schema like before, there will leave a new ledger for the schema. But with `-f`, all ledgers will be removed.
One place the ledgers of the schema shown is `internal-stats`, with or without `-f`, we will get a different result.
> Or delete forcefully should only be used to recover unexpected state (like your case, schema data deleted by offline process).
Delete forcefully should be used if we want to delete schema completely or to recover an unexpected state
--
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