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/02/06 02:17:52 UTC
[GitHub] [pulsar] rdhabalia opened a new pull request #9501: [pulsar-metadata] return alreadyExist error-code when node exists
rdhabalia opened a new pull request #9501:
URL: https://github.com/apache/pulsar/pull/9501
### Motivation
Right now, MetadataStore returns `BadVersion` error when node already exists instead `AlreadyExists` error-code. There are many usecases which require correct error code to handle failures. for example: [partitioned-topic](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L291) considers `AlreadyExists` error as successful result but instead metadata-api returns `BadVersion` error which creates complication while handling failures.
So, return correct error-code(`AlreadyExists`) if node already exists while creating metadata key-placeholder,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #9501: [pulsar-metadata] return alreadyExist error-code when node exists
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #9501:
URL: https://github.com/apache/pulsar/pull/9501#discussion_r571465395
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java
##########
@@ -213,7 +214,7 @@ public ZKMetadataStore(ZooKeeper zkc) {
future.complete(new Stat(name, 0, 0, 0));
} else if (code == Code.NODEEXISTS) {
// We're emulating a request to create node, so the version is invalid
- future.completeExceptionally(getException(Code.BADVERSION, path));
+ future.completeExceptionally(getException(Code.NODEEXISTS, path));
Review comment:
This was done on purpose here. Since there is no "create()" operation but just the `put()` with the optional expected version. A `create()` is equivalent to a `put(Optional.of(-1))` (eg: expecting the node not to be there). In that light it makes sense to me to stick to the BadVersion.
If the caller wants to make sure to create an entry, it just need to treat the bad version error for what it is: the state it's not what was expected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia closed pull request #9501: [pulsar-metadata] return alreadyExist error-code when node exists
Posted by GitBox <gi...@apache.org>.
rdhabalia closed pull request #9501:
URL: https://github.com/apache/pulsar/pull/9501
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on a change in pull request #9501: [pulsar-metadata] return alreadyExist error-code when node exists
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on a change in pull request #9501:
URL: https://github.com/apache/pulsar/pull/9501#discussion_r574840078
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java
##########
@@ -213,7 +214,7 @@ public ZKMetadataStore(ZooKeeper zkc) {
future.complete(new Stat(name, 0, 0, 0));
} else if (code == Code.NODEEXISTS) {
// We're emulating a request to create node, so the version is invalid
- future.completeExceptionally(getException(Code.BADVERSION, path));
+ future.completeExceptionally(getException(Code.NODEEXISTS, path));
Review comment:
we can handle `BADVERSION` as `NODEEXISTS` at caller side. but my only concern was what exact benefit we can get by returning different error-code. because actual error (in this case AlreadyExist) can help better in handling failure rather than deriving from `BADVERSION` . I will try to change handling at caller side but I think we might have usecases in future to explicitly know specific error to handle the failure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #9501: [pulsar-metadata] return alreadyExist error-code when node exists
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #9501:
URL: https://github.com/apache/pulsar/pull/9501#issuecomment-774517204
> partitioned-topic considers AlreadyExists error as successful result but instead metadata-api returns BadVersion error which creates complication while handling failures.
In this specific case, you're trying to create an entry. If you do `put(Optional.of(-1L)`, the only reason you'd get the BadVersion is if the entry is already there, so if that is ok, we can just ignore the BadVersion error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org