[jira] [Commented] (ZOOKEEPER-2695) Handle unknown error for rolling upgrade old client new server scenario

["Damien Diederen (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-2695?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17368712#comment-17368712 ] 

Damien Diederen commented on ZOOKEEPER-2695:
--------------------------------------------

Hi [~arshad.mohammad],

I was looking into this, following [this question|https://github.com/apache/zookeeper/pull/1716#pullrequestreview-687896698] of [~eolivelli]'s:

bq. You are also introducing a new error code, how do old clients will handle that error code?
This is a recurrent question—one to which I feel we don't have a very good answer.

The patch attached to this ticket still applies, provided one updates the paths and the target JUnit API imports. \(I am attaching a refreshed version as I have it at hand; feel free to turn it into a GitHub pull request.)

The proposed solution definitely improves on the status quo, but I have a few questions:

# {{Code.SYSTEMERROR}} is documented as follows:
bq. This is never thrown by the server, it shouldn't be used other than to indicate a range. Specifically error codes greater than this value, but lesser than {{#APIERROR}}, are system errors.
So I suppose I would suggest throwing {{SystemErrorException}} for codes in {{\[Code.SYSTEMERROR, Code.APIERROR)}} and {{APIErrorException}} codes outside that range. What do you think?
# Unfortunately, "known" system errors such as {{RuntimeInconsistencyException}} do not inherit from {{SystemErrorException}}. Similarly, an "API error" such as {{NoNodeException}} does not inherit from {{APIErrorException}}.
The list of {{KeeperException}} subclasses being flat means that clients have intimate knowledge of error codes to distinguish between exceptions which warrant an immediate retry, exceptions which should trigger exponential back-off, or fatal ones such as {{Code.AUTHFAILED}}.
While reparenting the existing classes would technically be an API/ABI break, we could potentially introduce superclasses such as e.g. {{BaseSystemErrorException}} between {{KeeperException}} and its children \(AFAICT, Sun & Oracle frequently do so), and use ranges to map unknown codes.
Is that something we should consider?
# {{KeeperException}} currently uses a {{private Code code}} member variable, which makes it impossible to propagate an unknown error codes.
Should we change that \(back?) to an {{int}}—without changing the public interface?
Of course, methods such as {{public Code code()}} would still have to return {{null}}, but others such as {{public int getCode()}} \(currently deprecated) could return the actual value, and an informative message could still be generated in {{public String getMessage()}}.

Thoughts?

> Handle unknown error for rolling upgrade old client new server scenario
> -----------------------------------------------------------------------
>
>                 Key: ZOOKEEPER-2695
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2695
>             Project: ZooKeeper
>          Issue Type: Bug
>          Components: java client
>            Reporter: Mohammad Arshad
>            Assignee: Mohammad Arshad
>            Priority: Major
>         Attachments: 0001-ZOOKEEPER-2695-Handle-unknown-error-for-rolling-upgr.patch, ZOOKEEPER-2695-01.patch
>
>
> In Zookeeper rolling upgrade scenario where server is new but client is old, when sever sends error code which is not understood by the client, client throws NullPointerException. 
> KeeperException.SystemErrorException should be thrown for all unknown error code.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)