You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by anmolnar <gi...@git.apache.org> on 2018/03/28 15:53:42 UTC
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
GitHub user anmolnar opened a pull request:
https://github.com/apache/zookeeper/pull/497
ZOOKEEPER-2415. SessionTest is using Thread deprecated API
Refactored `SessionTest` class.
`testSessionTimeout()` has been extracted to a separate class and split into multiple tests to validate the different scenarios in a more readable way.
Removed deprecated Thread API `suspend()` and reduced the server ticktime and session timeout for better performance.
Hopefully this will help on test's flakyness too.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2415
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/497.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #497
----
commit 64d5af67c4eeb34b6a06c057b5ecf0cdeb4b11df
Author: Andor Molnar <an...@...>
Date: 2018-03-28T15:50:15Z
ZOOKEEPER-2415. Refactor testSessionTimeout() to live in separate class and not to use deprecated API. Also improved performance.
----
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
Created https://issues.apache.org/jira/browse/ZOOKEEPER-3026 for followup on my ReadOnlyModeTest comment above.
---
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/497#discussion_r181526076
--- Diff: src/java/test/org/apache/zookeeper/test/SessionTimeoutTest.java ---
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.DisconnectableZooKeeper;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+public class SessionTimeoutTest extends ZKTestCase {
--- End diff --
Is there a reason why we can't use ClientBase instead of ZKTestCase?
---
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/497#discussion_r181525743
--- Diff: src/java/test/org/apache/zookeeper/test/SessionTimeoutTest.java ---
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.DisconnectableZooKeeper;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+public class SessionTimeoutTest extends ZKTestCase {
+ protected static final Logger LOG = LoggerFactory.getLogger(SessionTest.class);
--- End diff --
Should be SessionTimeoutTest rather than SessionTest.
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
+1. lgtm thanks @anmolnar !
---
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/497
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/497
@phunt Added a few more cleanups. I've found another test case in ZookeeperTestableTest class which also tests session expiration in a more reliable way. Therefore I've merged it into this new test class which also had the benefits of removing event/send thread getters.
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/497
+1.
Well done refactor of the testcase.
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
+1. Looks reasonable however:
1) this problem also exists in src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java - do you want to fix it as part of this change or handle as a separate jira? (either way is fine with me - separate might make it simpler to review/commit and make progress tbh, but up to you)
2) regardless 1) above, this doesn't apply to branch-3.4 - the original jira says it affects 3.4, I'm assuming we want to fix there as well? Could you create a separate PR for this? I'd like to commit both at the same time.
---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/497
@phunt Patch has been rebased and refactored to use ClientBase as base class.
---