You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Jay Shrauner <ja...@fb.com> on 2012/08/01 00:05:54 UTC
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
Re: Review Request: Multi-thread CommitProcessor
Posted by Jay Shrauner <ja...@fb.com>.
-----------------------------------------------------------
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
Posted by Jay Shrauner <ja...@fb.com>.
-----------------------------------------------------------
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
Posted by Jay Shrauner <ja...@fb.com>.
> 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
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
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
>
>