kezhuw commented on PR #2069: URL: https://github.com/apache/zookeeper/pull/2069#issuecomment-1742005015
> IIRC the sync() operation ensures that the server you are connected to is up-to-date and then you can read(). This works well when you write to ZK to some peer, then from another node of your application you read from a different ZK peer. It is not guaranteed currently in case of leader change. [QuorumSyncTest](https://github.com/apache/zookeeper/pull/2069/files#diff-9b8fbed71342b292e8293bd5a045fae256e16f4d685e2d753a88decf2ff0e15b) will fail if you change `followersProtocolVersion < ProtocolVersion.V3_10_0` to true. > If we change what sync() does at the moment and make it more heavyweight we are going to break applications, in production, because the load on ZK will increase. > This is something that you would see only in production underload, because developers working locally won't notice the difference. Two cents from my side. 1. Comparing to `getData`, `setData` and other data tree operations, `sync` is relatively rare. 2. This implementation will only issue a quorum operation when there is no outstanding proposal. So the cluster is probably not in a heavy load. In leader with outstanding proposals, `sync` will block on last proposal as before. I think the purposes are clear when people resort to `sync` + `read`, that is up-to-date data irrespective of cluster state(e.g. leader change). Comparing to this, I think the increasing in operation latency and cluster load when cluster has no outstanding proposal are probably acceptable. > In any case the client must explicitly opt-im to the new sync(). Though, I am not positive to this road, but I guess we can resort to a per-client option, say `quorumSync`, in `ZooKeeperBuilder`(ZOOKEEPER-4697) (#2001) in case we are going this way finally. Client side `sync` can issue different operations based on that option. The main doubt from me is that why people are intentionally want `sync` + `read` in case of them know `quorumSync` + `read` and the fault(not up-to-date data) of `sync` + `read` ? From my perspective, it is a bug for `sync` + `read` to read not up-to-date data. > We can discuss on the ML about why you need this I will prepare a discussion thread in dev mailing list later. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
