Re: Review Request: Multi-thread CommitProcessor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/ --- (Updated Oct. 12, 2012, 11:47 p.m.) Review request for zookeeper and Patrick Hunt. Changes --- Address feedback from review--shutdown CommitProcessor if downstream processor throws an exception (preserves previous behavior) Description --- See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 This addresses bug ZOOKEEPER-1505. https://issues.apache.org/jira/browse/ZOOKEEPER-1505 Diffs (updated) - /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1391526 /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1391526 /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1391526 /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1391526 /src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java PRE-CREATION Diff: https://reviews.apache.org/r/6260/diff/ Testing --- Thanks, Jay Shrauner
Re: Review Request: Multi-thread CommitProcessor
On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: This looks very nice Jay! I've looked through most of the code but not yet the CP logic itself. My thoughts so far: afaict the approach seems sound. Needs updates to the documentation. Needs unit tests to verify the new cases. Added unit test that tests the different configuration scenarios (0, 1, or many worker threads). Tightened restrictions on concurrency to prevent bug Thawan discovered (reads that reset watch in one session could race a write affecting the same node in another session). Updated related comments. On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, lines 460-474 https://reviews.apache.org/r/6260/diff/1/?file=131594#file131594line460 Is this a bug fix? If so it should be separated out to another jira and a test should be added for it. (likely we'd want to fix it in 3.3/3.4/trunk) You're right, this is an unrelated bug fix, I pulled it out. On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/WorkerService.java, line 56 https://reviews.apache.org/r/6260/diff/1/?file=131596#file131596line56 make this configurable. how did you come to 5 seconds as the default? Made it configurable. 5s was picked somewhat arbitrarily; I'm open to changing the default if you think some other value sounds more reasonable. On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 60 https://reviews.apache.org/r/6260/diff/1/?file=131597#file131597line60 convert these to javadoc so they show up in eclipse tools tips Done On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/WorkerService.java, lines 76-78 https://reviews.apache.org/r/6260/diff/1/?file=131596#file131596line76 move the method specific docs to the javadoc of the methods themselves. Done On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/WorkerService.java, line 203 https://reviews.apache.org/r/6260/diff/1/?file=131596#file131596line203 seems you need to be a bit careful when calling stop, if schedule() is already past the stoped check you could end up with a RejectedExecutionException being thrown? However in this case it seems only CommitProcessor shutdown is calling this.. Line 126 catches any RejectedExecutionExceptions being thrown and does cleanup On Aug. 2, 2012, 12:27 a.m., Patrick Hunt wrote: /src/java/main/org/apache/zookeeper/server/WorkerService.java, line 62 https://reviews.apache.org/r/6260/diff/1/?file=131596#file131596line62 doc this can be 0, and any other implications? Added - Jay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/#review9710 --- On July 31, 2012, 10:05 p.m., Jay Shrauner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/ --- (Updated July 31, 2012, 10:05 p.m.) Review request for zookeeper and Patrick Hunt. Description --- See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 This addresses bug ZOOKEEPER-1505. https://issues.apache.org/jira/browse/ZOOKEEPER-1505 Diffs - /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1366784 /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1366784 Diff: https://reviews.apache.org/r/6260/diff/ Testing --- Thanks, Jay Shrauner
Re: Review Request: Multi-thread CommitProcessor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/ --- (Updated Aug. 22, 2012, 9:20 p.m.) Review request for zookeeper and Patrick Hunt. Changes --- Addressed comments, added unit test. Bugfix discovered by Thawan. Tightened concurrency allowed: now a write transaction is not allowed to be run concurrently with reads from other sessions to prevent race condition with watch resets. Description --- See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 This addresses bug ZOOKEEPER-1505. https://issues.apache.org/jira/browse/ZOOKEEPER-1505 Diffs (updated) - /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1373156 /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1373156 /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1373156 /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1373156 /src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java PRE-CREATION Diff: https://reviews.apache.org/r/6260/diff/ Testing --- Thanks, Jay Shrauner
Re: Review Request: Multi-thread CommitProcessor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/#review9710 --- This looks very nice Jay! I've looked through most of the code but not yet the CP logic itself. My thoughts so far: afaict the approach seems sound. Needs updates to the documentation. Needs unit tests to verify the new cases. /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java https://reviews.apache.org/r/6260/#comment20711 Is this a bug fix? If so it should be separated out to another jira and a test should be added for it. (likely we'd want to fix it in 3.3/3.4/trunk) /src/java/main/org/apache/zookeeper/server/WorkerService.java https://reviews.apache.org/r/6260/#comment20716 make this configurable. how did you come to 5 seconds as the default? /src/java/main/org/apache/zookeeper/server/WorkerService.java https://reviews.apache.org/r/6260/#comment20718 doc this can be 0, and any other implications? /src/java/main/org/apache/zookeeper/server/WorkerService.java https://reviews.apache.org/r/6260/#comment20717 move the method specific docs to the javadoc of the methods themselves. /src/java/main/org/apache/zookeeper/server/WorkerService.java https://reviews.apache.org/r/6260/#comment20719 seems you need to be a bit careful when calling stop, if schedule() is already past the stoped check you could end up with a RejectedExecutionException being thrown? However in this case it seems only CommitProcessor shutdown is calling this.. /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java https://reviews.apache.org/r/6260/#comment20714 convert these to javadoc so they show up in eclipse tools tips - Patrick Hunt On July 31, 2012, 10:05 p.m., Jay Shrauner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/ --- (Updated July 31, 2012, 10:05 p.m.) Review request for zookeeper and Patrick Hunt. Description --- See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 This addresses bug ZOOKEEPER-1505. https://issues.apache.org/jira/browse/ZOOKEEPER-1505 Diffs - /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1366784 /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1366784 Diff: https://reviews.apache.org/r/6260/diff/ Testing --- Thanks, Jay Shrauner
Review Request: Multi-thread CommitProcessor
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6260/ --- Review request for zookeeper and Patrick Hunt. Description --- See https://issues.apache.org/jira/browse/ZOOKEEPER-1505 This addresses bug ZOOKEEPER-1505. https://issues.apache.org/jira/browse/ZOOKEEPER-1505 Diffs - /src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1366784 /src/java/main/org/apache/zookeeper/server/WorkerService.java PRE-CREATION /src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1366784 /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1366784 Diff: https://reviews.apache.org/r/6260/diff/ Testing --- Thanks, Jay Shrauner
