> On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line > > 147 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845237#file845237line147> > > > > Why interrupt() here? > > Add comments please.
When we catch and swallow InterruptedException, one common pattern is - call Thread.currentThread().interrupt() afterward, so that the interrupt status isn't lost. > On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java, > > line 46 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845240#file845240line46> > > > > thName -> threadName Ok > On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java, > > line 47 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845240#file845240line47> > > > > The logging is duplicate to that in notifyStopping(). > > > > I think we should remove the one in notifyStopping() because we might > > want to print exception stack. ofcourse, will remove duplicates > On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java, > > line 48 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845240#file845240line48> > > > > this.getName() -> threadName ok > On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java, > > line 168 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845249#file845249line168> > > > > Isn't "running" set to false in super.shutdown()? Yes, it synchronization should be improved further. Initially I thought of minimizing the scope of synchronized block, but I think it will not work out. Now, am planning to make both #startup() and #shutdown() synchronized methods. Any comments? synchronized startup() //... } synchronized shutdown(){ if(!isRunning()){ return; } super.shutdown(); // ... } > On Feb. 5, 2015, 6:50 p.m., Hongchao Deng wrote: > > ./src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java, > > line 135 > > <https://reviews.apache.org/r/20071/diff/4-5/?file=845252#file845252line135> > > > > Same.. it's always false. sure, will change this logic. - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20071/#review71262 ----------------------------------------------------------- On Feb. 5, 2015, 6:32 p.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20071/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2015, 6:32 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 > 1657623 > ./src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java 1657623 > ./src/java/main/org/apache/zookeeper/server/SyncRequestProcessor.java > 1657623 > ./src/java/main/org/apache/zookeeper/server/ZooKeeperCriticalThread.java > 1657623 > ./src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 1657623 > ./src/java/main/org/apache/zookeeper/server/ZooKeeperServerListener.java > PRE-CREATION > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/LeaderSessionTracker.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java > 1657623 > ./src/java/main/org/apache/zookeeper/server/quorum/LocalSessionTracker.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/ProposalRequestProcessor.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java > 1657623 > > ./src/java/main/org/apache/zookeeper/server/quorum/UpgradeableSessionTracker.java > 1657623 > ./src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java > 1657623 > ./src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 1657623 > ./src/java/test/org/apache/zookeeper/server/ZooKeeperThreadTest.java > 1657623 > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java > 1657623 > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java > 1657623 > ./src/java/test/org/apache/zookeeper/test/ClientBase.java 1657623 > ./src/java/test/org/apache/zookeeper/test/SessionTrackerCheckTest.java > 1657623 > > Diff: https://reviews.apache.org/r/20071/diff/ > > > Testing > ------- > > yet to be inlcuded > > > Thanks, > > Rakesh R > >
