Retries, Exceptions, Zookeeper API update

[ryanuber]

Let me start by saying that I am fully aware that this pull request may not align with the goals you have for the project, and that's ok. I already had most of this code implemented and figured I'd share it, and instead of forking I thought I'd try this first and solicit some feedback.

Ability to specify max tries per request

Networks have blips and other minor hiccups that can be dealt with by simply retrying the operation, especially in read operations or other idempotent tasks. This handy convenience feature would significantly improve the reliability of the library, as Zookeeper is very sensitive to its environment and can easily be affected by IOPS, system load, etc. Other Zookeeper client libraries have similar features, and I thought php-zookeeper deserved one as well.

This implementation allows for 3 new arguments for each query method that is supported in the library:

max_tries The maximum number of times to try the query

delay The initial delay (in milliseconds) between retries

backoff The multiplication factor to use between retries. If you started with 1000ms initial delay, 3 retries, and a backoff of 2, in a total failure scenario, the following would happen:

1) Try query and fail 2) Wait 1000ms 3) Tries query and fail 4) Wait 2000ms 5) Try query and fail 6) Raise error

The defaults for these parameters are set up so that the traditional one-try- only functionality is preserved (max_tries default=1).

Throw exceptions when things go wrong

I realize that this effort was started and discussed in #16. However, I think that it is important that the exceptions can easily describe what is actually happening to allow the programmer to decide what to do in any given error scenario.

For example, if I get a ZOPERATIONTIMEOUT, I might write the transaction to a log and try to replay it later on, whereas if I get a ZNONODE, I might want to report what the problem is to the user and return failure.

We don't need to use exceptions for flow control (exists() could still return false when the node doesn't exist, etc), but when Zookeeper does not do what you asked it to do for any reason, I would think an exception should be thrown in that case, accurately describing the reason for the failure.

Exceptions also really help when writing wrapper methods, since the exception handling can be delegated to wherever it needs to go, versus passing around return codes to be interpreted potentially multiple times.

This implementation of exceptions includes a few to start with:

ZookeeperException This is the base exception thrown if no other exception matches

ZookeeperOperationTimeoutException Thrown when a ZOPERATIONTIMEOUT is encountered

ZookeeperConnectionException Thrown when a ZCONNECTIONLOSS occurs, when the initial connection fails, or when a call to Zookeeper->connect() is not made before attempting a query.

ZookeeperMarshallingException Thrown when a ZMARSHALLINGERROR is encountered.

ZookeeperAuthenticationException Thrown when either a ZNOAUTH or ZAUTHFAILED is encountered.

ZookeeperSessionException Thrown when either a ZSESSIONEXPIRED or ZSESSIONMOVED is encountered.

ZookeeperNoNodeException Thrown when a ZNONODE exception occurs. This can actually be an error condition if the end user is trying to do a set() or delete() on a znode which does not exist. Of course a special case is added for exists() to check for ZNONODE before throwing an exception.

Updated Zookeeper API to 3.5.x

Since Zookeeper is a very conservative project in terms of release timelines, at my workplace we have been using Zookeeper 3.5.0 for a little over a month now and haven't seen any major problems. This update changes Zookeeper's LOG() API syntax, requiring a callback to be passed in. I have updated all of the LOG() (and variants) calls here to handle that. This is optional and could be rolled back, since it is a backward-incompatible change.

So let me know what you think. I'm interested in hearing any actionable suggestions or opinions out there.

[r3nat]

:+1:

[andreiz]

Hi, I need a bit of time to review this. Should be able to merge later early next week.

[faabiosr]

Well! I'm think is great this ideia to throw exceptions and retries, but this pull-request is very big, if you change the aproach to split into two PRs?

A PR for exception and another to retries, and the first one is retries, thus does not break the semantic versioning.

[ryanuber]

Thanks for the replies. Things have changed since I wrote this code over a year ago, and I'm not using PHP in any projects currently, so I probably won't be able to get back into this.

@fabiorphp it's possible this could be split into multiple PR's, but honestly I'm not sure if that would help. ~300 lines isn't an enormous amount to review. I'm also unsure of what you mean by breaking the semantic versioning. The changes in this PR update the Zookeeper API version and changes function signatures, which are backwards-incompatible changes, thus the bump I did to 0.3.0. In semver, pre-1.0 is like the wild west anyways (no compatibility promise), so I guess it doesn't matter - feel free to change that.

If anyone wants to pick this up, feel free to copy the code out and modify it, create separate PR's, or anything else.

Best of luck, and thanks for the library @andreiz! It was useful.

[faabiosr]

Thanks @ryanuber!

[faabiosr]

How we could adjust this pull-request to merge with master branch?