> On Jan. 26, 2015, 5:32 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line > > 159 > > <https://reviews.apache.org/r/20071/diff/3/?file=834241#file834241line159> > > > > Is there any race in listener between run() and shutdown()? Maybe some > > comments? > > Rakesh R wrote: > yes. I missed that part. How about making listener as 'volatile' ? > > Hongchao Deng wrote: > It's a problem called time-to-check-time-to-use. > > Rakesh R wrote: > Then will move the notification inside exception-handling block of each > thread. I'm just trying to avoid extra synchronized block. agree? > > Rakesh R wrote: > Adding one more point to the above. My intention to make listener=null > is, the listener should not be invoked when the thread stops execution > gracefully(through #shutdown call). > > Hongchao Deng wrote: > Adding a synchronized block seems fine to me actually. We don't bother to > care about efficiency in shutting down. But making the code have good > concurrency practice has more long term benefit. > > synchronizing over the exit code, or the singleton listener class, or > anything you can think of in any processor class is good here.
I've moved the listerner object to the ZKCriticalThread and notified in the exception block. Does this sound good to you? > On Jan. 26, 2015, 5:32 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java, line 430 > > <https://reviews.apache.org/r/20071/diff/3/?file=834244#file834244line430> > > > > This should be a singleton, right? > > Rakesh R wrote: > yes, I will make it singleton. private final ZooKeeperServerListener listener = new ZooKeeperServerListenerImpl(); Set the listener at ZooKeeperServer level. ok? - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20071/#review69622 ----------------------------------------------------------- On Jan. 26, 2015, 2:12 p.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20071/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2015, 2:12 p.m.) > > > Review request for zookeeper, michim, Raul Gutierrez Segales, and Camille > Fournier. > > > Bugs: ZOOKEEPER-1907 > https://issues.apache.org/jira/browse/ZOOKEEPER-1907 > > > Repository: zookeeper > > > Description > ------- > > Improve the thread handling mechanism by detecting if any of the critical > thread dies. > Here the idea is to periodically checking the status of all the critical > threads in ZK server using DeathWatcherThread. > > > Diffs > ----- > > ./src/java/main/org/apache/zookeeper/server/ExitCode.java PRE-CREATION > ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java > 1654804 > ./src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java 1654804 > ./src/java/main/org/apache/zookeeper/server/SyncRequestProcessor.java > 1654804 > ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1654804 > ./src/java/main/org/apache/zookeeper/server/ZooKeeperServerListener.java > PRE-CREATION > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/LeaderSessionTracker.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java > 1654804 > ./src/java/main/org/apache/zookeeper/server/quorum/LocalSessionTracker.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/ProposalRequestProcessor.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java > 1654804 > > ./src/java/main/org/apache/zookeeper/server/quorum/UpgradeableSessionTracker.java > 1654804 > ./src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java > 1654804 > ./src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 1654804 > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java > 1654804 > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java > 1654804 > ./src/java/test/org/apache/zookeeper/test/ClientBase.java 1654804 > ./src/java/test/org/apache/zookeeper/test/SessionTrackerCheckTest.java > 1654804 > > Diff: https://reviews.apache.org/r/20071/diff/ > > > Testing > ------- > > yet to be inlcuded > > > Thanks, > > Rakesh R > >
