You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Rakesh R <ra...@huawei.com> on 2014/03/28 19:42:54 UTC
Review Request 19794: PurgeTxnLog may delete data logs during roll
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/
-----------------------------------------------------------
Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
Bugs: ZOOKEEPER-1797
https://issues.apache.org/jira/browse/ZOOKEEPER-1797
Repository: zookeeper
Description
-------
PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
Diffs
-----
./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1581683
./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1581683
Diff: https://reviews.apache.org/r/19794/diff/
Testing
-------
I've tried to add a tests
Thanks,
Rakesh R
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review38994
-----------------------------------------------------------
Ship it!
Ship It!
- michim
On March 29, 2014, 7:23 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated March 29, 2014, 7:23 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On May 1, 2014, 12:24 p.m., Bill Havanki wrote:
> > Hi there!
> >
> > (I'm new to ZK, and I'm checking out some code reviews to get acquainted with the project.)
> >
> > The unit test here is good, but it doesn't appear to check that the code selects the correct set of files to purge, but that the process at least completes. It would be good to break out the logic for selecting the files into a separate method and then testing that with mock data for the variety of snapshot and log files available. It'd be easy to test edge cases that way, too. I see that Mockito is already a ZK dependency, and that can definitely do it. I'd be happy to help out with creating such a test.
>
> Rakesh R wrote:
> Thank you Bill, its really good idea to test as smaller units. I've tried an approach without using mockito. Could you look at it ?
>
> Note: For testing purpose, I've created new method visible for testing PurgeTxnLog#retainNRecentSnapshots()
Hi Rakesh,
The new tests look great! They definitely do the job of verifying that the right files are purged.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review41921
-----------------------------------------------------------
On May 2, 2014, 11:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated May 2, 2014, 11:07 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1591929
> ./src/java/test/org/apache/zookeeper/server/PurgeTxnTest.java PRE-CREATION
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1591929
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Rakesh R <ra...@huawei.com>.
> On May 1, 2014, 4:24 p.m., Bill Havanki wrote:
> > Hi there!
> >
> > (I'm new to ZK, and I'm checking out some code reviews to get acquainted with the project.)
> >
> > The unit test here is good, but it doesn't appear to check that the code selects the correct set of files to purge, but that the process at least completes. It would be good to break out the logic for selecting the files into a separate method and then testing that with mock data for the variety of snapshot and log files available. It'd be easy to test edge cases that way, too. I see that Mockito is already a ZK dependency, and that can definitely do it. I'd be happy to help out with creating such a test.
Thank you Bill, its really good idea to test as smaller units. I've tried an approach without using mockito. Could you look at it ?
Note: For testing purpose, I've created new method visible for testing PurgeTxnLog#retainNRecentSnapshots()
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review41921
-----------------------------------------------------------
On March 29, 2014, 7:23 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated March 29, 2014, 7:23 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review41921
-----------------------------------------------------------
Hi there!
(I'm new to ZK, and I'm checking out some code reviews to get acquainted with the project.)
The unit test here is good, but it doesn't appear to check that the code selects the correct set of files to purge, but that the process at least completes. It would be good to break out the logic for selecting the files into a separate method and then testing that with mock data for the variety of snapshot and log files available. It'd be easy to test edge cases that way, too. I see that Mockito is already a ZK dependency, and that can definitely do it. I'd be happy to help out with creating such a test.
- Bill Havanki
On March 29, 2014, 3:23 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated March 29, 2014, 3:23 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/
-----------------------------------------------------------
(Updated May 2, 2014, 3:07 p.m.)
Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
Changes
-------
Addressing Bill's comment. Updated new patch by including few more unit tests which contains the following changes:
1) created new method visible for testing PurgeTxnLog#retainNRecentSnapshots()
2) Added 4 new test cases.
3) PurgeTxnTest class moved to 'org.apache.zookeeper.server' package
Bugs: ZOOKEEPER-1797
https://issues.apache.org/jira/browse/ZOOKEEPER-1797
Repository: zookeeper
Description
-------
PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1591929
./src/java/test/org/apache/zookeeper/server/PurgeTxnTest.java PRE-CREATION
./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1591929
Diff: https://reviews.apache.org/r/19794/diff/
Testing
-------
I've tried to add a tests
Thanks,
Rakesh R
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/
-----------------------------------------------------------
(Updated March 29, 2014, 7:23 a.m.)
Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
Changes
-------
Thank you Michi for the reviews. Attached latest patch addressing the review comments.
Bugs: ZOOKEEPER-1797
https://issues.apache.org/jira/browse/ZOOKEEPER-1797
Repository: zookeeper
Description
-------
PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962
./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962
Diff: https://reviews.apache.org/r/19794/diff/
Testing
-------
I've tried to add a tests
Thanks,
Rakesh R
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by Rakesh R <ra...@huawei.com>.
> On March 29, 2014, 12:16 a.m., michim wrote:
> > ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java, line 56
> > <https://reviews.apache.org/r/19794/diff/1/?file=539875#file539875line56>
> >
> > nit: trailing space
done
> On March 29, 2014, 12:16 a.m., michim wrote:
> > ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java, line 78
> > <https://reviews.apache.org/r/19794/diff/1/?file=539875#file539875line78>
> >
> > We should probably define final strings for "snapshot" and "log".
done
> On March 29, 2014, 12:16 a.m., michim wrote:
> > ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java, line 97
> > <https://reviews.apache.org/r/19794/diff/1/?file=539876#file539876line97>
> >
> > nit: trailing space
done
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review38971
-----------------------------------------------------------
On March 29, 2014, 7:23 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated March 29, 2014, 7:23 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1582962
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1582962
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19794: PurgeTxnLog may delete data logs during roll
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19794/#review38971
-----------------------------------------------------------
The patch looks great overall. I just had a few minor nitpicks.
./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java
<https://reviews.apache.org/r/19794/#comment71351>
nit: trailing space
./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java
<https://reviews.apache.org/r/19794/#comment71356>
We should probably define final strings for "snapshot" and "log".
./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java
<https://reviews.apache.org/r/19794/#comment71352>
nit: trailing space
- michim
On March 28, 2014, 6:42 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19794/
> -----------------------------------------------------------
>
> (Updated March 28, 2014, 6:42 p.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> Bugs: ZOOKEEPER-1797
> https://issues.apache.org/jira/browse/ZOOKEEPER-1797
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> PurgeTxnLog may delete data logs if logs are rolling or a new snapshot is created during this process
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/PurgeTxnLog.java 1581683
> ./src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java 1581683
>
> Diff: https://reviews.apache.org/r/19794/diff/
>
>
> Testing
> -------
>
> I've tried to add a tests
>
>
> Thanks,
>
> Rakesh R
>
>