You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Hongchao Deng <hd...@cloudera.com> on 2014/10/08 07:34:54 UTC
Review Request 26437: ZooKeeper-2052
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Marshall McMullen <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/#review56660
-----------------------------------------------------------
Ship it!
Ship It!
- Marshall McMullen
On Oct. 8, 2014, 9:18 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26437/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 9:18 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZooKeeper-2052
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
> src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
> src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
>
> Diff: https://reviews.apache.org/r/26437/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 15, 2014, 5:22 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 15, 2014, 4:30 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/#review56665
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment97075>
Just a minor suggestion, can we simply return it like.
synchronized (zks.outstandingChanges) {
return zks.outstandingChangesForPath.get(path);
}
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
<https://reviews.apache.org/r/26437/#comment97069>
Just a suggestion. Please add null checks for zks and servcnxnf. In case of any failures it may give NPE and create confusions.
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
<https://reviews.apache.org/r/26437/#comment97074>
Removed the assertion, I understood the reason now many are using the MyRequestProcessor. But its good to retain the MARSHALLINGERROR code verification which is used by 'testPRequest' case.
One idea is to create a separate MyRequestProcessor1 for this testcase or if you have any better idea, always welcome.
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
<https://reviews.apache.org/r/26437/#comment97076>
Instead of sleeping and checking the znode existence, can we modify by watching the ephemral znode deletion and then do multi execution ?
- Rakesh R
On Oct. 8, 2014, 9:18 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26437/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 9:18 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZooKeeper-2052
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
> src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
> src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
>
> Diff: https://reviews.apache.org/r/26437/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 26437: ZooKeeper-2052
Posted by Marshall McMullen <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/#review56658
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment97061>
Thanks for adding this comment here.
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
<https://reviews.apache.org/r/26437/#comment97062>
Really good additional tests. Nice job.
- Marshall McMullen
On Oct. 8, 2014, 9:18 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26437/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 9:18 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZooKeeper-2052
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
> src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
> src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
>
> Diff: https://reviews.apache.org/r/26437/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 9:18 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
> On Oct. 8, 2014, 9:06 p.m., Raul Gutierrez Segales wrote:
> > src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java, line 269
> > <https://reviews.apache.org/r/26437/diff/4/?file=715636#file715636line269>
> >
> > why?
The test might be flaky without this sleep: epheZk closed, process closeSession request; meanwhile zk send a getData request.
My intention is not to make this test flaky. What do you think?
- Hongchao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/#review55875
-----------------------------------------------------------
On Oct. 8, 2014, 6:33 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26437/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:33 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZooKeeper-2052
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
> src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
> src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
>
> Diff: https://reviews.apache.org/r/26437/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 26437: ZooKeeper-2052
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/#review55875
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment96245>
Nit: this method name is weird... maybe just getOutstandingChange or even if too long getOutstandingChangeForPath is better i think.
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment96246>
nit: *existing
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment96247>
nit: *for paths
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
<https://reviews.apache.org/r/26437/#comment96248>
nit: existing
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
<https://reviews.apache.org/r/26437/#comment96249>
nit: drop this line
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java
<https://reviews.apache.org/r/26437/#comment96250>
nit: ditto
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
<https://reviews.apache.org/r/26437/#comment96252>
nit: i would drop the the 'expected exception' comments... the intent is clear.
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
<https://reviews.apache.org/r/26437/#comment96251>
why?
- Raul Gutierrez Segales
On Oct. 8, 2014, 6:33 p.m., Hongchao Deng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26437/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:33 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> ZooKeeper-2052
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
> src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
> src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
>
> Diff: https://reviews.apache.org/r/26437/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 6:33 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 6:30 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng
Re: Review Request 26437: ZooKeeper-2052
Posted by Hongchao Deng <hd...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26437/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 6:28 p.m.)
Review request for zookeeper.
Repository: zookeeper-git
Description
-------
ZooKeeper-2052
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 8542790
src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8caf419
src/java/test/org/apache/zookeeper/test/ClientBase.java a6229b5
src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java a573180
Diff: https://reviews.apache.org/r/26437/diff/
Testing
-------
Thanks,
Hongchao Deng