[
https://issues.apache.org/jira/browse/ZOOKEEPER-2619?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15602873#comment-15602873
]
Diego Ongaro commented on ZOOKEEPER-2619:
-----------------------------------------
Thanks for the quick replies.
bq. In your first example, you're mixing the synchronous and the asynchronous
API. We don't actually enforce that an application uses one or the other, but
we do not recommend to mix synchronous and asynchronous calls precisely because
of the reasons you're raising.
In my three-line example, it doesn't matter whether the second create is
synchronous (mixing) or asynchronous (not mixing). Isn't it fine to issue a
series of asynchronous calls ending with a synchronous call?
Summarizing the *{{reenableOps()}}* proposal so far, we have:
- A new configuration option (say, "requireReenableOps") for an application to
opt into calling {{reenableOps()}}. This would preserve backwards compatibility
for existing clients, so the default would be to keep today's behavior.
- Upon a connection loss, the client library would still reconnect
automatically and issue pings as needed. However, with the configuration option
set, it would continue to fail all requests (both synchronous and asynchronous).
- After the application calls {{reenableOps()}}, the client library would
permit new requests to use the new connection.
This would work, but in terms of API design, I can think of a couple of
drawbacks:
- If we consider an application that's spread across various modules and/or
thread boundaries, it has to be carefully designed with respect to
{{reenableOps()}}, as this is a global property. I think this is what [~breed]
was getting at with "it affects everything using the zookeeper handle".
Specifically, every module/thread must be ready for a call to {{reenableOps()}}
before it is safe to invoke, and until such time, the application won't make
much progress.
- Most asynchronous code will need to know whether it's running under
requireReenableOps=false or requireReenableOps=true. This might complicate a
reusable library's internals and API, which may need to have different behavior
depending on this setting.
The *{{getConnection()}}* proposal doesn't have these problems. Some modules of
an application can continue issuing synchronous calls on the ZooKeeper object,
others can issue (deprecated) asynchronous calls on the ZooKeeper object, and
yet others can use ZKConnection objects, moving to new connections at their own
pace with no application-level synchronization.
bq. it sounds to me that getConnection() and reenableOps() are basically the
same. right? or are you proposing that when you get a ZKConnection object you
can invoke the zookeeper operations on that?
You would invoke the operations directly on the ZKConnection object.
I do like the idea of the library automatically reconnecting for pings, and we
could apply that to the {{getConnection()}} proposal too:
- The library would reconnect automatically to issue pings, as it does now.
- {{ZooKeeper.getConnection()}} would return the current ZKConnection object,
which wraps the current TCP connection.
- Upon a connection loss, all subsequent operations on the same ZKConnection
would return a Connection Loss error. The library would internally reconnect
automatically, but it would not make this TCP connection availble via the old
ZKConnection.
- Upon noticing a connection loss, the application would need to call
{{ZooKeeper.getConnection()}} again to get the new ZKConnection object.
The ZKConnection object certainly needs to export the asynchronous methods that
are in ZooKeeper today. I think it should also export the synchronous methods.
I think these are both well-defined and useful in ZKConnection, as in my
original example with {{createAsync()}} followed by {{createSync()}}.
I think the ZooKeeper class should continue to export non-deprecated
synchronous methods as well. These would just be convenience wrappers that
invoke {{getConnection()}} followed by the synchronous operation on that. Much
ZooKeeper code is entirely synchronous, and as such, does not depend on FIFO
client order. I see no reason to force churn upon that code.
I still think the asynchronous calls on the ZooKeeper class should be
deprecated, since applications cannot rely on FIFO client order when using
them. At a minimum, these should have a very clear warning to this effect, and
applications should be encouraged to get a ZKConnection and then make
asynchronous calls on that.
> Client library reconnecting breaks FIFO client order
> ----------------------------------------------------
>
> Key: ZOOKEEPER-2619
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2619
> Project: ZooKeeper
> Issue Type: Bug
> Reporter: Diego Ongaro
>
> According to the USENIX ATC 2010
> [paper|https://www.usenix.org/conference/usenix-atc-10/zookeeper-wait-free-coordination-internet-scale-systems],
> ZooKeeper provides "FIFO client order: all requests from a given client are
> executed in the order that they were sent by the client." I believe
> applications written using the Java client library are unable to rely on this
> guarantee, and any current application that does so is broken. Other client
> libraries are also likely to be affected.
> Consider this application, which is simplified from the algorithm described
> on Page 4 (right column) of the paper:
> {code}
> zk = new ZooKeeper(...)
> zk.createAsync("/data-23857", "...", callback)
> zk.createSync("/pointer", "/data-23857")
> {code}
> Assume an empty ZooKeeper database to begin with and no other writers.
> Applying the above definition, if the ZooKeeper database contains /pointer,
> it must also contain /data-23857.
> Now consider this series of unfortunate events:
> {code}
> zk = new ZooKeeper(...)
> // The library establishes a TCP connection.
> zk.createAsync("/data-23857", "...", callback)
> // The library/kernel closes the TCP connection because it times out, and
> // the create of /data-23857 is doomed to fail with ConnectionLoss. Suppose
> // that it never reaches the server.
> // The library establishes a new TCP connection.
> zk.createSync("/pointer", "/data-23857")
> // The create of /pointer succeeds.
> {code}
> That's the problem: subsequent operations get assigned to the new connection
> and succeed, while earlier operations fail.
> In general, I believe it's impossible to have a system with the following
> three properties:
> # FIFO client order for asynchronous operations,
> # Failing operations when connections are lost, AND
> # Transparently reconnecting when connections are lost.
> To argue this, consider an application that issues a series of pipelined
> operations, then upon noticing a connection loss, issues a series of recovery
> operations, repeating the recovery procedure as necessary. If a pipelined
> operation fails, all subsequent operations in the pipeline must also fail.
> Yet the client must also carry on eventually: the recovery operations cannot
> be trivially failed forever. Unfortunately, the client library does not know
> where the pipelined operations end and the recovery operations begin. At the
> time of a connection loss, subsequent pipelined operations may or may not be
> queued in the library; others might be upcoming in the application thread. If
> the library re-establishes a connection too early, it will send pipelined
> operations out of FIFO client order.
> I considered a possible workaround of having the client diligently check its
> callbacks and watchers for connection loss events, and do its best to stop
> the subsequent pipelined operations at the first sign of a connection loss.
> In addition to being a large burden for the application, this does not solve
> the problem all the time. In particular, if the callback thread is delayed
> significantly (as can happen due to excessive computation or scheduling
> hiccups), the application may not learn about the connection loss event until
> after the connection has been re-established and after dependent pipelined
> operations have already been transmitted over the new connection.
> I suggest the following API changes to fix the problem:
> - Add a method ZooKeeper.getConnection() returning a ZKConnection object.
> ZKConnection would wrap a TCP connection. It would include all synchronous
> and asynchronous operations currently defined on the ZooKeeper class. Upon a
> connection loss on a ZKConnection, all subsequent operations on the same
> ZKConnection would return a Connection Loss error. Upon noticing, the client
> would need to call ZooKeeper.getConnection() again to get a working
> ZKConnection object, and it would execute its recovery procedure on this new
> connection.
> - Deprecate all asynchronous methods on the ZooKeeper object. These are
> unsafe to use if the caller assumes they're getting FIFO client order.
> - No changes to the protocols or servers are required.
> I recognize this could cause a lot of code churn for both ZooKeeper and
> projects that use it. On the other hand, the existing asynchronous calls in
> applications should now be audited anyhow.
> The code affected by this issue may be difficult to contain:
> - It likely affects all ZooKeeper client libraries that provide both
> asynchronous operations and transparent reconnection. That's probably all
> versions of the official Java client library, as well as most other client
> libraries.
> - It affects all applications using those libraries that depend on the FIFO
> client order of asynchronous operations. I don't know how common that is, but
> the paper implies that FIFO client order is important.
> - Fortunately, the issue can only manifest itself when connections are lost
> and transparently reestablished. In practice, it may also require a long
> pipeline or a significant delay in the application thread while the library
> establishes a new connection.
> - In case you're wondering, this issue occurred to me while working on a new
> client library for Go. I haven't seen this issue in the wild, but I was able
> to produce it locally by placing sleep statements in a Java program and
> closing its TCP connections.
> I'm new to this community, so I'm looking forward to the discussion. Let me
> know if I can clarify any of the above.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
