Skip to content

Conversation

@rdhabalia
Copy link
Contributor

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 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,

@rdhabalia rdhabalia added this to the 2.8.0 milestone Feb 6, 2021
@rdhabalia rdhabalia self-assigned this Feb 6, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

} 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@merlimat
Copy link
Contributor

merlimat commented Feb 6, 2021

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.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @merlimat 's comment.

In fact there was a comment explaining the reason of the BADVERSION code.

the weird fact is that ZK API returns a NODEEXISTS error.

We are abstracting from ZK API, so we can have our own behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants