You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Kfir Lev-Ari <kf...@gmail.com> on 2014/08/28 20:27:58 UTC
Review Request 25160: Major throughput improvement with mixed
workloads
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 7, 2014, 9:39 p.m., Alexander Shraer wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 519
> > <https://reviews.apache.org/r/25160/diff/2/?file=675058#file675058line519>
> >
> > indentation
(I dropped all the issues that aren't found at version 3.)
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52561
-----------------------------------------------------------
On Sept. 8, 2014, 5:18 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2014, 5:18 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 7, 2014, 9:39 p.m., Alexander Shraer wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 86
> > <https://reviews.apache.org/r/25160/diff/2/?file=675058#file675058line86>
> >
> > Please move comment to preceding line (try to have ~80 chars per line also elsewhere), and add one more sentence saying what's the idea here.
This parameter was removed, please see the updated version in which all ready read requests are processed on the same round.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52561
-----------------------------------------------------------
On Sept. 3, 2014, 8:52 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 8:52 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 7, 2014, 9:39 p.m., Alexander Shraer wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 202
> > <https://reviews.apache.org/r/25160/diff/2/?file=675058#file675058line202>
> >
> > indentation
Not sure if I got this one right, please verify.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52561
-----------------------------------------------------------
On Sept. 8, 2014, 7:42 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2014, 7:42 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52561
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91380>
Please move comment to preceding line (try to have ~80 chars per line also elsewhere), and add one more sentence saying what's the idea here.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91381>
could you add an explanation about the states here ?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91379>
why not have HashMap<Long, LinkedList<Request>> instead ? you're not processing the sub-lists concurrently, so you could just process the head of the list until reaching the next write (or the end).
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91384>
add: indexed by session id
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91382>
please fix indentation
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91383>
indentation
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91385>
please add a comment to explain this
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91386>
please add a comment to explain this line
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91388>
can you add an explanation here?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91389>
Have -> Has
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91390>
indentation
- Alexander Shraer
On Sept. 3, 2014, 8:52 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2014, 8:52 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On פבר' 13, 2016, 10:05 אחר הצהריים, Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java, line 224
> > <https://reviews.apache.org/r/25160/diff/16/?file=1240919#file1240919line224>
> >
> > isnt this a read only workload (#writes = 0) ?
Yes, but every client performs a single write (first operation).
> On פבר' 13, 2016, 10:05 אחר הצהריים, Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java, line 249
> > <https://reviews.apache.org/r/25160/diff/16/?file=1240919#file1240919line249>
> >
> > why should it be <= numClients ?
Should be == numClients (each client performs a single write)
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119163
-----------------------------------------------------------
On פבר' 16, 2016, 7:04 בבוקר, Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated פבר' 16, 2016, 7:04 בבוקר)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119163
-----------------------------------------------------------
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 217)
<https://reviews.apache.org/r/25160/#comment180446>
isnt this a read only workload (#writes = 0) ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 242)
<https://reviews.apache.org/r/25160/#comment180447>
why should it be <= numClients ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 267)
<https://reviews.apache.org/r/25160/#comment180449>
why should it be <= numClients ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 303)
<https://reviews.apache.org/r/25160/#comment180450>
why is this needed ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 306)
<https://reviews.apache.org/r/25160/#comment180451>
why is this needed ?
- Alexander Shraer
On Feb. 12, 2016, 7:22 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2016, 7:22 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119280
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 217)
<https://reviews.apache.org/r/25160/#comment180593>
add "for client session id ..."
to the log message
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 286)
<https://reviews.apache.org/r/25160/#comment180602>
this needs more explanation. Can you add comments and/or test plan ? I actually liked the fact that in the previous version of this test you had a lot of committed writes and all was missing seemed to be to check that the number of iterations to clear the reads depends on the number of reads and not the number of committed writes. This would show they're not blocked by the committed writes. Here you have less of both and its not clear what's happening or whether this tests no starvation. Also, the last check seems insufficient - reads may be in pending blocked behind something, check that they were done.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 217)
<https://reviews.apache.org/r/25160/#comment180606>
can you please add a comment about this here
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 242)
<https://reviews.apache.org/r/25160/#comment180607>
in that case please change to == and add a comment
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java (line 267)
<https://reviews.apache.org/r/25160/#comment180608>
same as before
- Alexander Shraer
On Feb. 16, 2016, 7:04 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2016, 7:04 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119810
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 193)
<https://reviews.apache.org/r/25160/#comment181161>
Isn't this a duplicate of lines 205 - 208 ?
Maybe you can remove it from here along with the comment
- Alexander Shraer
On Feb. 19, 2016, 3:55 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2016, 3:55 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119818
-----------------------------------------------------------
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 88)
<https://reviews.apache.org/r/25160/#comment181166>
can you simplify this ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 126)
<https://reviews.apache.org/r/25160/#comment181167>
not necessarily read, change the variable name
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 132)
<https://reviews.apache.org/r/25160/#comment181168>
Briefly explain test plan.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 220)
<https://reviews.apache.org/r/25160/#comment181169>
wrong error message
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 242)
<https://reviews.apache.org/r/25160/#comment181170>
queueRequests -> queuedRequests
- Alexander Shraer
On Feb. 19, 2016, 6:02 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2016, 6:02 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated אפריל 12, 2016, 6:55 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Set an initial hash size of 10K sessions. (Please ignore revision 29)
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated אפריל 12, 2016, 6:26 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 96
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line96>
> >
> > Could you align the lines of the comment pls?
Done.
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 85
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line85>
> >
> > Why is this not final any longer?
Because the test assigns a mock queue to this variable. See "processor.queuedRequests = new MockRequestsQueue();" in CommitProcessorConcurrencyTest.java
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 165
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line165>
> >
> > These comment lines are too long, please respect the 80 character limit.
Done
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 172
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line172>
> >
> > More long lines...
Done
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 190
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line190>
> >
> > Can we maintain the comment style /* */, pls?
Done
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 201
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line201>
> >
> > You can call just pendingRequests.put(request.sessionId, new LinkedList<Request>()); instead and remove the previous line.
requests is updated since it is used below this code.
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 223
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line223>
> >
> > Why do we have to wait for empty pool both here and line 245?
This is the same as before the patch (check out the condition for waking up the thread right now).
Due to synchronization issues in FinalRequestProcessor (potential races between setting watches and updating data), commitRequestProcessor
has always either sent it many reads or a single write. We kept this in the patch. I think this part can be made more efficient,
e.g., using read/write locks, and I plan to open a separate Jira for this.
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 405
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315548#file1315548line405>
> >
> > This wakeupOnEmpty() call is synchronizing on a different object compared to wakeup(). I'm not entirely clear on how the synchronization goes here because it looks like we are now synchronizing on both this and emptyPoolSync.
https://goo.gl/m1cINJ gives more details, please see pseudocode on the last page. The idea is that synchronization on
new incoming requests is now separate from synchronization on "thread pool is available to process more requests", since
we may have work to do even if there is no "new" incoming requests.
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, line 151
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315549#file1315549line151>
> >
> > Long line.
Done
> On March 27, 2016, 2:42 p.m., fpj wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, line 166
> > <https://reviews.apache.org/r/25160/diff/28/?file=1315549#file1315549line166>
> >
> > Long line.
Done
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125565
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125565
-----------------------------------------------------------
The patch looks mostly good. I have several trivial comments, but some around synchronization that I want to understand better. I haven't looked at the tests, though, but I'll leave it for later.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 84)
<https://reviews.apache.org/r/25160/#comment188437>
Why is this not final any longer?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 95)
<https://reviews.apache.org/r/25160/#comment188438>
Could you align the lines of the comment pls?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 155)
<https://reviews.apache.org/r/25160/#comment188432>
These comment lines are too long, please respect the 80 character limit.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 162)
<https://reviews.apache.org/r/25160/#comment188439>
More long lines...
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 177)
<https://reviews.apache.org/r/25160/#comment188433>
Can we maintain the comment style /* */, pls?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 188)
<https://reviews.apache.org/r/25160/#comment188436>
You can call just pendingRequests.put(request.sessionId, new LinkedList<Request>()); instead and remove the previous line.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 210)
<https://reviews.apache.org/r/25160/#comment188441>
Why do we have to wait for empty pool both here and line 245?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 337)
<https://reviews.apache.org/r/25160/#comment188440>
This wakeupOnEmpty() call is synchronizing on a different object compared to wakeup(). I'm not entirely clear on how the synchronization goes here because it looks like we are now synchronizing on both this and emptyPoolSync.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 150)
<https://reviews.apache.org/r/25160/#comment188435>
Long line.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 165)
<https://reviews.apache.org/r/25160/#comment188434>
Long line.
- fpj
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated מרץ 26, 2016, 7:06 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Removed traling whitespaces
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 26, 2016, 6:49 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 231
> > <https://reviews.apache.org/r/25160/diff/27/?file=1315415#file1315415line231>
> >
> > shouldn't this be an assertion? if request is null here things went totally wrongn (before we'd just crash with a NullPointerException), but now we just retry? if it is indeed retryable, we shouldn't log this as an error.
We decided to only throw an exception for errors indicating an inconsistent state and for other errors just log and continue.
Do you think its better to throw an exception here and in the other place Rakesh commented on ?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125540
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 26, 2016, 6:49 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 231
> > <https://reviews.apache.org/r/25160/diff/27/?file=1315415#file1315415line231>
> >
> > shouldn't this be an assertion? if request is null here things went totally wrongn (before we'd just crash with a NullPointerException), but now we just retry? if it is indeed retryable, we shouldn't log this as an error.
>
> Alexander Shraer wrote:
> We decided to only throw an exception for errors indicating an inconsistent state and for other errors just log and continue.
> Do you think its better to throw an exception here and in the other place Rakesh commented on ?
Changed this to throw an exception.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125540
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125540
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 218)
<https://reviews.apache.org/r/25160/#comment188392>
shouldn't this be an assertion? if request is null here things went totally wrongn (before we'd just crash with a NullPointerException), but now we just retry? if it is indeed retryable, we shouldn't log this as an error.
- Raul Gutierrez Segales
On March 26, 2016, 5:37 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 5:37 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
> On March 26, 2016, 6:38 p.m., Raul Gutierrez Segales wrote:
> >
minor, though important, nit: mind dropping the extra whitespaces and tabs included? Thanks!
- Raul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125537
-----------------------------------------------------------
On March 26, 2016, 5:37 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 5:37 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 26, 2016, 6:38 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 99
> > <https://reviews.apache.org/r/25160/diff/27/?file=1315415#file1315415line99>
> >
> > hmm, should preallocate memory for this hashmap? imagine this scenario (which i've seen in production):
> >
> > * you have > 100K sessions
> > * a very bad partition happens
> > * after a long election clients start reconnecting creating and reading more than 200k ephemerals znodes and setting more than 600k watches
> >
> > wonder how reallocating this hashmap as those requests come in would work...
Sounds like a good idea, what would be a good initial size ? given that not everyone run ZK with som many client sessions...
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125537
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 26, 2016, 6:38 p.m., Raul Gutierrez Segales wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 99
> > <https://reviews.apache.org/r/25160/diff/27/?file=1315415#file1315415line99>
> >
> > hmm, should preallocate memory for this hashmap? imagine this scenario (which i've seen in production):
> >
> > * you have > 100K sessions
> > * a very bad partition happens
> > * after a long election clients start reconnecting creating and reading more than 200k ephemerals znodes and setting more than 600k watches
> >
> > wonder how reallocating this hashmap as those requests come in would work...
>
> Alexander Shraer wrote:
> Sounds like a good idea, what would be a good initial size ? given that not everyone run ZK with som many client sessions...
Initialized to 10000 for now. If you have better suggestions let me know.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125537
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125537
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 98)
<https://reviews.apache.org/r/25160/#comment188387>
hmm, should preallocate memory for this hashmap? imagine this scenario (which i've seen in production):
* you have > 100K sessions
* a very bad partition happens
* after a long election clients start reconnecting creating and reading more than 200k ephemerals znodes and setting more than 600k watches
wonder how reallocating this hashmap as those requests come in would work...
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 106)
<https://reviews.apache.org/r/25160/#comment188383>
nit: trailing whitespace
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 129)
<https://reviews.apache.org/r/25160/#comment188384>
nit: trailing whitespace (or tab?)
- Raul Gutierrez Segales
On March 26, 2016, 5:37 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 5:37 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated מרץ 26, 2016, 5:37 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Extracted "wait for pool" to a separate method.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On מרץ 25, 2016, 9:05 בבוקר, Rakesh R wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 269
> > <https://reviews.apache.org/r/25160/diff/26/?file=1259184#file1259184line269>
> >
> > Should the admin take any action on seeing this error? I'm interested to know the severity of this state, say monitoring tool or admin detect this error message.
This check (request != null) should never be true and may indicate a bug, so we log it.
If it is detected the code will skip the request instead of crashing with null pointer as before.
This error doesn't indicate an inconsistent state so we decided to just skip the request.
> On מרץ 25, 2016, 9:05 בבוקר, Rakesh R wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 285
> > <https://reviews.apache.org/r/25160/diff/26/?file=1259184#file1259184line285>
> >
> > Here its throwing exception and exits CommitProcessor. Could you please explain me about this case and the reason to exit rather than failing the request alone.
This error indicates an inconsistency since we know we lost an operation, so we decided to throw an exception instead of silently skipping it.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125396
-----------------------------------------------------------
On מרץ 26, 2016, 5:37 בבוקר, Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated מרץ 26, 2016, 5:37 בבוקר)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Rakesh R <ra...@intel.com>.
> On March 25, 2016, 9:05 a.m., Rakesh R wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 269
> > <https://reviews.apache.org/r/25160/diff/26/?file=1259184#file1259184line269>
> >
> > Should the admin take any action on seeing this error? I'm interested to know the severity of this state, say monitoring tool or admin detect this error message.
>
> Kfir Lev-Ari wrote:
> This check (request != null) should never be true and may indicate a bug, so we log it.
> If it is detected the code will skip the request instead of crashing with null pointer as before.
> This error doesn't indicate an inconsistent state so we decided to just skip the request.
>
> Alexander Shraer wrote:
> Following Raul's comment, changing this to throw an exception.
OK, agreed.
> On March 25, 2016, 9:05 a.m., Rakesh R wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 285
> > <https://reviews.apache.org/r/25160/diff/26/?file=1259184#file1259184line285>
> >
> > Here its throwing exception and exits CommitProcessor. Could you please explain me about this case and the reason to exit rather than failing the request alone.
>
> Kfir Lev-Ari wrote:
> This error indicates an inconsistency since we know we lost an operation, so we decided to throw an exception instead of silently skipping it.
ok, fine.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125396
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On March 25, 2016, 9:05 a.m., Rakesh R wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 269
> > <https://reviews.apache.org/r/25160/diff/26/?file=1259184#file1259184line269>
> >
> > Should the admin take any action on seeing this error? I'm interested to know the severity of this state, say monitoring tool or admin detect this error message.
>
> Kfir Lev-Ari wrote:
> This check (request != null) should never be true and may indicate a bug, so we log it.
> If it is detected the code will skip the request instead of crashing with null pointer as before.
> This error doesn't indicate an inconsistent state so we decided to just skip the request.
Following Raul's comment, changing this to throw an exception.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125396
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review125396
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 223)
<https://reviews.apache.org/r/25160/#comment188191>
Should the admin take any action on seeing this error? I'm interested to know the severity of this state, say monitoring tool or admin detect this error message.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 239)
<https://reviews.apache.org/r/25160/#comment188184>
Here its throwing exception and exits CommitProcessor. Could you please explain me about this case and the reason to exit rather than failing the request alone.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 250)
<https://reviews.apache.org/r/25160/#comment188185>
How about extracting this to a separate method as this logic is duplciated twice.
- Rakesh R
On Feb. 19, 2016, 12:56 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2016, 12:56 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 19, 2016, 12:56 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 19, 2016, 6:02 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 19, 2016, 3:55 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 18, 2016, 7:01 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 18, 2016, 1:45 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 17, 2016, 9:07 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119463
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 158)
<https://reviews.apache.org/r/25160/#comment180829>
Please add a comment saying that Since requests are placed in the queue before being sent to the leader, if commitIsWaiting = true, the commit belongs to one of the first requestsToProcess operations in the queue or to a request from a client on another server (order of the following two lines is important!).
Add that in each iteration we process up to requestsToProcess requests from the incoming queue (possibly less if a committed request can be matched to a pending local write) and one committed request if commitIsWaiting.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 184)
<https://reviews.apache.org/r/25160/#comment180830>
add that this is because committs for local requests arrive in the order they appeared in the queue, so if we have something in pending and a committed request, the committed request must be for that pending write or for a write originating at a different server
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 229)
<https://reviews.apache.org/r/25160/#comment180835>
could you add several commits ? with just one its possible that we're just processing it first
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 287)
<https://reviews.apache.org/r/25160/#comment180832>
there are 100 requests in this loop already. Do we need the previous single commit ? if so please update the test plan to say 101 commits
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 301)
<https://reviews.apache.org/r/25160/#comment180834>
Can you invoke twice without the loop and check state after each invocation ?
Can you check that the rest of the committed requests remain in the committed queue and are not in the processedRequests
- Alexander Shraer
On Feb. 17, 2016, 2:11 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2016, 2:11 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 17, 2016, 2:11 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Now without starved non-local committed requests
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119336
-----------------------------------------------------------
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 252)
<https://reviews.apache.org/r/25160/#comment180649>
I suggest to have 50 reads instead of 5 in the last step, and have the 100 committed updates only in committed, not in queue. Then check that it takes <= 50 runs to remove the reads from the queue and that after that there are still some committed updates in committed, i.e., reads were not starved by committed.
Also, could you add another test to check that committed aren't starved by reads ? This is the bug you found and described in the jira, so we should have a test for it if we don't already. I think just as above - have many more reads in queue than committed in committed, and check that all committed are processed and many reads remain in queue.
- Alexander Shraer
On Feb. 16, 2016, 11:44 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2016, 11:44 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 16, 2016, 11:44 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 16, 2016, 11:38 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 16, 2016, 7:04 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On פבר' 13, 2016, 6:27 אחר הצהריים, Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, line 216
> > <https://reviews.apache.org/r/25160/diff/16/?file=1240918#file1240918line216>
> >
> > also check that all the following reads were executed
You mean reads in shouldBeProcessedAfterPending?
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119151
-----------------------------------------------------------
On פבר' 16, 2016, 7:04 בבוקר, Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated פבר' 16, 2016, 7:04 בבוקר)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
> On Feb. 13, 2016, 6:27 p.m., Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java, line 216
> > <https://reviews.apache.org/r/25160/diff/16/?file=1240918#file1240918line216>
> >
> > also check that all the following reads were executed
>
> Kfir Lev-Ari wrote:
> You mean reads in shouldBeProcessedAfterPending?
yes
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119151
-----------------------------------------------------------
On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated March 26, 2016, 7:06 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review119151
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 164)
<https://reviews.apache.org/r/25160/#comment180426>
The red spaces here and elsewhere are probably tabs. Please try to remove
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 210)
<https://reviews.apache.org/r/25160/#comment180428>
Log an error here if topPending.cxid is different than request.cxid
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 215)
<https://reviews.apache.org/r/25160/#comment180427>
I'd move this to the end - you have this same code also on line 238, so group it all together in one place.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 233)
<https://reviews.apache.org/r/25160/#comment180429>
looks like if you remove !sessionQueue.isEmpty() from the if condition you'll be able to remove empty queues here also for the case that there aren't following reads in the session
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 275)
<https://reviews.apache.org/r/25160/#comment180430>
is this if needed ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 80)
<https://reviews.apache.org/r/25160/#comment180432>
why remove this ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 203)
<https://reviews.apache.org/r/25160/#comment180436>
also check that all the following reads were executed
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 207)
<https://reviews.apache.org/r/25160/#comment180437>
Please describe in the test plan what happens from this line onward.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 224)
<https://reviews.apache.org/r/25160/#comment180438>
check that the request in the queue is the write request
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 270)
<https://reviews.apache.org/r/25160/#comment180441>
where is ready used ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 281)
<https://reviews.apache.org/r/25160/#comment180440>
shouldn't this be 5 iterations ?
- Alexander Shraer
On Feb. 12, 2016, 7:22 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2016, 7:22 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 12, 2016, 7:22 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Fixed unit test
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 12, 2016, 7:05 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Refactored code structure to be more similar to original
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 12, 2016, 12:23 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Small ajustment (for read-only workloads) - no need to use isEmpty of LinkedBlockingQueue, if the private (non synchronized) HashMap is empty.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 11, 2016, 5:43 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Cleaner version for diff
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On פבר' 10, 2016, 9:59 אחר הצהריים, Chris Conroy wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 98
> > <https://reviews.apache.org/r/25160/diff/12/?file=1239815#file1239815line98>
> >
> > shouldn't this be a concurrent map OR by `@GaurdedBy` an appropriate lock?
since it is solely used by the commit processor thread, we don't need an additional synchronization here.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review118732
-----------------------------------------------------------
On פבר' 11, 2016, 5:43 בבוקר, Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated פבר' 11, 2016, 5:43 בבוקר)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Chris Conroy <cc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review118732
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 96)
<https://reviews.apache.org/r/25160/#comment180034>
shouldn't this be a concurrent map OR by `@GaurdedBy` an appropriate lock?
- Chris Conroy
On Feb. 10, 2016, 8:18 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 8:18 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated פבר' 10, 2016, 8:18 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Updated commit processor algorithm.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated יאנ' 26, 2016, 12:08 בבוקר)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Fixed unit test accordingly
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated יאנ' 25, 2016, 11:51 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Added a missing wait() call, this is important for performance
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated יאנ' 25, 2016, 10:53 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Simplified algorithm + updated for current version of ZK
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1726708
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1726708
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 11, 2014, 9:25 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Removed unneeded cast from old line 298 ((Map.Entry<Long, LinkedList<Request>>), and on the way moved function to match reading order.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated ספט' 10, 2014, 8:31 אחר הצהריים)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On ספט' 9, 2014, 6:14 אחר הצהריים, Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java, line 158
> > <https://reviews.apache.org/r/25160/diff/6/?file=683706#file683706line158>
> >
> > I'm not sure this does what you think it does. If I understand correctly your test ends when the teardown sets stop, not when TEST_RUN_TIME_IN_MS is over. I'm not sure but this looks like a timeout value.
Actually I used it this way in order to avoid the sessions timeouts caused by the session tracker. Anyway I'll add a mock session tracker instead (we just needed a different ID per session, so I'll add it to the mock).
> On ספט' 9, 2014, 6:14 אחר הצהריים, Alexander Shraer wrote:
> > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java, line 218
> > <https://reviews.apache.org/r/25160/diff/6/?file=683706#file683706line218>
> >
> > This would pass even if you processed no write requests. (similarly in other tests) Do you want all submitted requests to be processed ? perhaps you should track number of submitted requests at each client ? I guess depends on what you'd like to test.
> >
> > Also, consider extending the checkprocessedrequest method to get some parameters and check more stuff (you invoke it from some of the tests but not from others)
Just saying that the order between the requests per session is validated by the ValidateProcessor, and I thought that this is the main idea of the test. The reason for numClients here is that at most #numClients create requests should be processed, the rest are reads. Anyway, if I'll have an idea about how to improve this test, I'll add it.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52755
-----------------------------------------------------------
On ספט' 9, 2014, 8:51 בבוקר, Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated ספט' 9, 2014, 8:51 בבוקר)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52755
-----------------------------------------------------------
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
<https://reviews.apache.org/r/25160/#comment91795>
can you add an explanation regarding your run time parameter ?
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
<https://reviews.apache.org/r/25160/#comment91801>
I'm not sure this does what you think it does. If I understand correctly your test ends when the teardown sets stop, not when TEST_RUN_TIME_IN_MS is over. I'm not sure but this looks like a timeout value.
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
<https://reviews.apache.org/r/25160/#comment91783>
perhaps change to while (!stopped)
remove the check for stopped and move sleep to after sending the request
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
<https://reviews.apache.org/r/25160/#comment91794>
This would pass even if you processed no write requests. (similarly in other tests) Do you want all submitted requests to be processed ? perhaps you should track number of submitted requests at each client ? I guess depends on what you'd like to test.
Also, consider extending the checkprocessedrequest method to get some parameters and check more stuff (you invoke it from some of the tests but not from others)
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java
<https://reviews.apache.org/r/25160/#comment91786>
getFirstProcessor().start()
- Alexander Shraer
On Sept. 9, 2014, 8:51 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 8:51 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 9, 2014, 8:51 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Please compare revision 6 to 4, (I forgot to add CommitProcessorConcurrencyTest.java changes in 5).
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 9, 2014, 8:33 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 9, 2014, 5:48 a.m., Alexander Shraer wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 283
> > <https://reviews.apache.org/r/25160/diff/2-4/?file=675058#file675058line283>
> >
> > Instead of "addToPending" I suggest to call this function "addToRequestMap" or something similar, and it will called like addToRequestMap(readyReadRequests, request) and addToRequestMap(sessionPendings, request).
> >
> > You could then delete the "getSessionsRequests" function (btw, its called get... but changes state, which is not so nice)
> >
> > I suggest to rename "sessionPendings" to "pendingRequests" (to make it similar to queuedRequests and readyReadRequests).
Thanks, nice refactoring!
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52675
-----------------------------------------------------------
On Sept. 9, 2014, 8:33 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2014, 8:33 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Alexander Shraer <sh...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52675
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91615>
Instead of "addToPending" I suggest to call this function "addToRequestMap" or something similar, and it will called like addToRequestMap(readyReadRequests, request) and addToRequestMap(sessionPendings, request).
You could then delete the "getSessionsRequests" function (btw, its called get... but changes state, which is not so nice)
I suggest to rename "sessionPendings" to "pendingRequests" (to make it similar to queuedRequests and readyReadRequests).
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91617>
thanks for this explanation, it helps!
I suggest to move up the last part of the "if" (queuedRequests.isEmpty()) to match the order in your explanation
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91622>
you can probably remove this second isEmpty check - if its empty you'll find and remove it next round.
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment91630>
the is -> is the
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java
<https://reviews.apache.org/r/25160/#comment91634>
If I understand correctly, you're emptying the queuedRequests but this doesn't check that the comitted op is the one being processed.
- Alexander Shraer
On Sept. 8, 2014, 7:42 a.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2014, 7:42 a.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 8, 2014, 7:42 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 8, 2014, 5:18 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Changes
-------
Process all ready read requests on the same round.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/
-----------------------------------------------------------
(Updated Sept. 3, 2014, 8:52 a.m.)
Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
Repository: zookeeper
Description
-------
Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
Diff: https://reviews.apache.org/r/25160/diff/
Testing
-------
The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
Thanks,
Kfir Lev-Ari
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 2, 2014, 10:02 p.m., Grant Monroe wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 277
> > <https://reviews.apache.org/r/25160/diff/1/?file=671464#file671464line277>
> >
> > This looks controversial. I can see the advantage of fast forwarding pings to keep the client from disconnecting, but this means that pings no longer indicate the health of the write pipeline, i.e. before this patch, in the case of a pending write, pings are blocked. This patch changes that behavior.
I agree. At first it looked like the right thing to do, but now I think that removing this change is better in order to keep the patch as slim as possible. I guess that this change should be tested and discussed separately.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52091
-----------------------------------------------------------
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Grant Monroe <gr...@tnarg.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52091
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment90827>
This looks controversial. I can see the advantage of fast forwarding pings to keep the client from disconnecting, but this means that pings no longer indicate the health of the write pipeline, i.e. before this patch, in the case of a pending write, pings are blocked. This patch changes that behavior.
- Grant Monroe
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 2, 2014, 9:39 p.m., Grant Monroe wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 351
> > <https://reviews.apache.org/r/25160/diff/1/?file=671464#file671464line351>
> >
> > Should this be continue instead of return? If a single session has reached MAX_OUTSTANDING_READS_PER_SESSION, do we won't to stop processing other sessions?
Yes, good catch, it should be continue instead of return at the other lines in that loop as well.
> On Sept. 2, 2014, 9:39 p.m., Grant Monroe wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 281
> > <https://reviews.apache.org/r/25160/diff/1/?file=671464#file671464line281>
> >
> > I think this would be clearer as
> >
> > if (needCommit(request) || pendingRequests.keySet().contains(request.sessionId)) {
> > addToPending(request);
> > } else {
> > getSessionsRequests(request).addLast(request);
> > }
Agree.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52075
-----------------------------------------------------------
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Grant Monroe <gr...@tnarg.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52075
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment90825>
I think this would be clearer as
if (needCommit(request) || pendingRequests.keySet().contains(request.sessionId)) {
addToPending(request);
} else {
getSessionsRequests(request).addLast(request);
}
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment90826>
Should this be continue instead of return? If a single session has reached MAX_OUTSTANDING_READS_PER_SESSION, do we won't to stop processing other sessions?
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment90810>
s/outsanding/outstanding/g
- Grant Monroe
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Kfir Lev-Ari <kf...@gmail.com>.
> On Sept. 2, 2014, 8:23 p.m., Grant Monroe wrote:
> > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java, line 391
> > <https://reviews.apache.org/r/25160/diff/1/?file=671464#file671464line391>
> >
> > what if request is null?
Right, it'll be an error since we reach this code only based on the check that committed head isn't null, I'll add an error print + return.
- Kfir
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52074
-----------------------------------------------------------
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>
Re: Review Request 25160: Major throughput improvement with mixed
workloads
Posted by Grant Monroe <gr...@tnarg.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25160/#review52074
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
<https://reviews.apache.org/r/25160/#comment90808>
what if request is null?
- Grant Monroe
On Aug. 28, 2014, 6:27 p.m., Kfir Lev-Ari wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25160/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 6:27 p.m.)
>
>
> Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer.
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java 1619360
> ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java 1619360
>
> Diff: https://reviews.apache.org/r/25160/diff/
>
>
> Testing
> -------
>
> The attached unit tests, as well as the system test found in https://issues.apache.org/jira/browse/ZOOKEEPER-2023.
>
>
> Thanks,
>
> Kfir Lev-Ari
>
>