[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/529 ---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user timothyjward commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196731884
--- Diff: src/java/test/org/apache/zookeeper/test/WatcherTest.java ---
@@ -140,6 +142,55 @@ public void processResult(int rc, String path, Object
ctx) {
}
}
}
+
+@Test
+public void testWatcherDisconnectOnClose()
+throws IOException, InterruptedException, KeeperException
+{
+ZooKeeper zk = null;
+try {
+final BlockingQueue queue = new
LinkedBlockingQueue<>();
+
+MyWatcher connWatcher = new MyWatcher();
+
+Watcher watcher = new Watcher(){
+@Override
+public void process(WatchedEvent event) {
+try {
+queue.put(event);
+} catch (InterruptedException e) {
+// Oh well, never mind
+}
+}
+
+};
+
+zk = createClient(connWatcher, hostPort);
+
+StatCallback scb = new StatCallback() {
+public void processResult(int rc, String path, Object ctx,
+Stat stat) {
+// don't do anything
+}
+};
+
+// Register a watch on the node
+zk.exists("/missing", watcher, scb, null);
+
+// Close the client without changing the node
+zk.close();
+
+
+WatchedEvent event = queue.poll(10, TimeUnit.SECONDS);
+Assert.assertEquals(Event.EventType.None, event.getType());
--- End diff --
I've pushed updated assertions containing more human readable text.
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196712402
--- Diff: src/java/test/org/apache/zookeeper/test/WatcherTest.java ---
@@ -140,6 +142,55 @@ public void processResult(int rc, String path, Object
ctx) {
}
}
}
+
+@Test
+public void testWatcherDisconnectOnClose()
+throws IOException, InterruptedException, KeeperException
+{
+ZooKeeper zk = null;
+try {
+final BlockingQueue queue = new
LinkedBlockingQueue<>();
+
+MyWatcher connWatcher = new MyWatcher();
+
+Watcher watcher = new Watcher(){
+@Override
+public void process(WatchedEvent event) {
+try {
+queue.put(event);
+} catch (InterruptedException e) {
+// Oh well, never mind
+}
+}
+
+};
+
+zk = createClient(connWatcher, hostPort);
+
+StatCallback scb = new StatCallback() {
+public void processResult(int rc, String path, Object ctx,
+Stat stat) {
+// don't do anything
+}
+};
+
+// Register a watch on the node
+zk.exists("/missing", watcher, scb, null);
+
+// Close the client without changing the node
+zk.close();
+
+
+WatchedEvent event = queue.poll(10, TimeUnit.SECONDS);
+Assert.assertEquals(Event.EventType.None, event.getType());
--- End diff --
When your patch isn't applied, the watcher won't receive any event and
timeout occurs. Resulting in `event == null` and test will get
NullReferenceException. Would you please assert for null ref and also add some
meaningful messages to the assert statements which indicate what exactly went
wrong and what should be the expected behaviour.
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196256868
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1438,6 +1436,11 @@ public void disconnect() {
}
sendThread.close();
+try {
+sendThread.join();
+} catch (InterruptedException ex) {
+LOG.warn("Got interrupted while waiting for the sender thread
to close", ex);
+}
eventThread.queueEventOfDeath();
--- End diff --
Sounds good to me - thanks for running the experiment!
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user timothyjward commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196119877
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1438,6 +1436,11 @@ public void disconnect() {
}
sendThread.close();
+try {
+sendThread.join();
+} catch (InterruptedException ex) {
+LOG.warn("Got interrupted while waiting for the sender thread
to close", ex);
+}
eventThread.queueEventOfDeath();
--- End diff --
If the `queueEventOfDeath` call moves above the `join` call then some
events can be missed.
Specifically closing the connection's eventOfDeath can be processed before
completing the send of the session disconnection message triggered in
disconnect.
I tried moving the queueEventOfDeath above the join, but then some of the
core tests began failing intermittently. This was remedied by moving the
eventOfDeath back after the completion of the send thread.
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user timothyjward commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196102971
--- Diff: src/java/main/org/apache/zookeeper/Watcher.java ---
@@ -84,7 +84,14 @@
* client connection (the session) is no longer valid. You must
* create a new client connection (instantiate a new ZooKeeper
* instance) if you with to access the ensemble. */
-Expired (-112);
+Expired (-112),
+
+/**
+ * The client has been closed. This state is never generated by
+ * the server, but is generated locally when a client calls
+ * {@link ZooKeeper#close()} or {@link ZooKeeper#close(int)}
+ */
+Closed (-2);
--- End diff --
My understanding was that a negative value would be preferred for an event
which didn't originate from the server, but I don't mind changing it.
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user timothyjward commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r196090467
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1260,10 +1260,8 @@ public void run() {
cleanup();
}
clientCnxnSocket.close();
-if (state.isAlive()) {
-eventThread.queueEvent(new
WatchedEvent(Event.EventType.None,
-Event.KeeperState.Disconnected, null));
-}
+eventThread.queueEvent(new WatchedEvent(Event.EventType.None,
+Event.KeeperState.Closed, null));
--- End diff --
Good catch - in the previous version of this patch the terminal event
re-used the disconnected state for closing (hence removing the if check. Now
that we have a Closed event the Disconnected event should still be sent if the
client is connected at the time of the close.
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r195949397
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1438,6 +1436,11 @@ public void disconnect() {
}
sendThread.close();
+try {
+sendThread.join();
+} catch (InterruptedException ex) {
+LOG.warn("Got interrupted while waiting for the sender thread
to close", ex);
+}
eventThread.queueEventOfDeath();
--- End diff --
Any gains/problems with moving the queueEventOfDeath above the join()?
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r195949453
--- Diff: src/java/main/org/apache/zookeeper/Watcher.java ---
@@ -84,7 +84,14 @@
* client connection (the session) is no longer valid. You must
* create a new client connection (instantiate a new ZooKeeper
* instance) if you with to access the ensemble. */
-Expired (-112);
+Expired (-112),
+
+/**
+ * The client has been closed. This state is never generated by
+ * the server, but is generated locally when a client calls
+ * {@link ZooKeeper#close()} or {@link ZooKeeper#close(int)}
+ */
+Closed (-2);
--- End diff --
Nit - make the enum value 7?
---
[GitHub] zookeeper pull request #529: [ZOOKEEPER-2368] Send a watch event is when a c...
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/529#discussion_r195949390
--- Diff: src/java/main/org/apache/zookeeper/ClientCnxn.java ---
@@ -1260,10 +1260,8 @@ public void run() {
cleanup();
}
clientCnxnSocket.close();
-if (state.isAlive()) {
-eventThread.queueEvent(new
WatchedEvent(Event.EventType.None,
-Event.KeeperState.Disconnected, null));
-}
+eventThread.queueEvent(new WatchedEvent(Event.EventType.None,
+Event.KeeperState.Closed, null));
--- End diff --
I'd expect the client to always receive a Disconnected state before
receiving a Closed state and it looks like that constraint is not enforced.
---
