ZOOKEEPER-4968: Add interfaces to cover ZooKeeper client operations #2308
Conversation
kezhuw
force-pushed
the
ZOOKEEPER-4968-ZooKeeper-interfaces-to-cover-client-operations
branch
from
902d71c to
c0b40d1
Compare
| import org.apache.zookeeper.data.ClientInfo; | ||
| import org.apache.zookeeper.data.Stat; | ||
|
|
||
| class ZooKeeperAdaptor implements ZooKeeper { |
| @@ -79,7 +78,7 @@ public void testBuildClient() throws Exception { | |||
| @Test | |||
| public void testBuildAdminClient() throws Exception { | |||
| BlockingQueue<WatchedEvent> events = new LinkedBlockingQueue<>(); | |||
| ZooKeeper zk = new ZooKeeperBuilder(hostPort, Duration.ofMillis(1000)) | |||
| ZooKeeper zk = ZooKeeper.builder(hostPort, Duration.ofMillis(1000)) | |||
There was a problem hiding this comment.
Return interface should be ZooKeeperAdmin.
Changes: 1. Add new interface `Zookeeper` in `o.a.z.client`. 2. Add `ZooKeeper::builder` to construct instance of `ZooKeeper`. 3. Add `ZooKeeperAdaptor` to proxy interface methods to `o.a.z.ZooKeeper` instance to keep abi compatibility.
kezhuw
force-pushed
the
ZOOKEEPER-4968-ZooKeeper-interfaces-to-cover-client-operations
branch
from
c0b40d1 to
0b3e3cb
Compare
There was a problem hiding this comment.
What would be the benefit of this change ?
My initial motivations:
- Refactoring
ZooKeepertoo.a.zookeeper.clientin a smooth way to accomodate possible future migration to java module. By introducing interface ino.a.zookeeper.clientand also bridges toWatcher,WatchedEvent, etc., we can migrate all client side operation to packageo.a.zookeeper.clientwithout breaking anything. It might not be worth though if we are going to migrate to java module in one step without deprecation phase. - A chance to rethink/refactor some of apis. Say, for
getSessionIdto return typed session id with customtoStringor refactor watcher system to solve ZOOKEEPER-4625.
I think an interface for client side operations is also good for community as it is easy to do proxy/intercept/enhance things than concrete class, and more importantly it comes from official zookeeper package so the community could share their builds with each other.
Are we going to have a separate ZooKeeper client maven module at some point?
I have put effort on this. See:
|
|
||
| /** | ||
| * Adaptor to bridge {@link org.apache.zookeeper.ZooKeeper} to implement {@link ZooKeeper} while not introducing | ||
| * abi compatibility issue. |
There was a problem hiding this comment.
It is a breaking change in abi level to implement ZooKeeper for org.apache.zookeeper.ZooKeeper.
Adaptor is also good if we want to modify behavior slightly.
| * | ||
| * @since 3.5.0 | ||
| */ | ||
| void removeWatches( |
There was a problem hiding this comment.
This method is cheating since ZOOKEEPER-1910, see ZOOKEEPER-4625.
There was a problem hiding this comment.
The difference with the earlier api is,
(1)this won't remove the given watcher from the server set even if the watcher set is empty, but it will remove the reference from the zkclient. This api will invoke a call to server and maintain orderly execution as earlier. IMHO sending to server is required to handle packet in-flight cases.
(2)null watcher is not allowed, client will validate and throw IllegalArgumentException.
We are now have persistent watcher now, it will continue event delivery until removed. If we are going to use new interface, it is a chance for us to reevaluate this api.
| * | ||
| * @return current session id | ||
| */ | ||
| long getSessionId(); |
There was a problem hiding this comment.
It would be nice to return typed session id with custom toString, so we don't have to use Long.toHexString everytime in logging.
Changes:
Zookeeperino.a.z.client.ZooKeeper::builderto construct instance ofZooKeeper.ZooKeeperAdaptorto proxy interface methods too.a.z.ZooKeeperinstance to keep abi compatibility.