You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by ke...@deenlo.com on 2014/03/21 20:27:04 UTC
Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-2520
https://issues.apache.org/jira/browse/ACCUMULO-2520
Repository: accumulo
Description
-------
This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
Diffs
-----
src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
Diff: https://reviews.apache.org/r/19546/diff/
Testing
-------
Added unit test
Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
Thanks,
kturner
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by Sean Busbey <se...@manvsbeard.com>.
> On March 21, 2014, 7:46 p.m., Sean Busbey wrote:
> > Can you include a functional test that replicates the catastrophic failure that prompted ACCUMULO-2520 to verify that this change stops it?
>
> kturner wrote:
> Like a functional test? Changes in the unit test cover this case, but not in an end to end way.
Yeah, given the severity of the failure mode, I'd really like an end-to-end. I also think a functional test is more likely to catch a future regression.
On March 21, 2014, 7:46 p.m., kturner wrote:
> > Once this is forward ported to 1.6.0-SNAP, will it have to switch to absolute paths? both absolute and relative?
> >
>
> kturner wrote:
> I did a good bit of refactoring of the GC as part of ACCUMULO-1773 to make it more testable and to handle abs/rel paths. I also added more sanity checks to the GC. I have not yet determined if those checks will cover this case. At the very least 1.6.0 will need to add new test.
>
> I think the diffs for 1.6.0 will be very different. I was thinking that once these changes are reviewed, that maybe I could post another set of diffs for 1.6.0. Not sure what the best way to handle this is. I wanted to wait on merging this change forward locally in case some major issue came up in the review.
That sounds reasonable.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38173
-----------------------------------------------------------
On March 21, 2014, 7:27 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:27 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by ke...@deenlo.com.
> On March 21, 2014, 7:46 p.m., Sean Busbey wrote:
> > Can you include a functional test that replicates the catastrophic failure that prompted ACCUMULO-2520 to verify that this change stops it?
Like a functional test? Changes in the unit test cover this case, but not in an end to end way.
> On March 21, 2014, 7:46 p.m., Sean Busbey wrote:
> > src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java, lines 442-443
> > <https://reviews.apache.org/r/19546/diff/1/?file=531749#file531749line442>
> >
> > nit: would you mind including {} for the if block?
sure
On March 21, 2014, 7:46 p.m., kturner wrote:
> > Once this is forward ported to 1.6.0-SNAP, will it have to switch to absolute paths? both absolute and relative?
> >
I did a good bit of refactoring of the GC as part of ACCUMULO-1773 to make it more testable and to handle abs/rel paths. I also added more sanity checks to the GC. I have not yet determined if those checks will cover this case. At the very least 1.6.0 will need to add new test.
I think the diffs for 1.6.0 will be very different. I was thinking that once these changes are reviewed, that maybe I could post another set of diffs for 1.6.0. Not sure what the best way to handle this is. I wanted to wait on merging this change forward locally in case some major issue came up in the review.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38173
-----------------------------------------------------------
On March 21, 2014, 7:27 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:27 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38173
-----------------------------------------------------------
Can you include a functional test that replicates the catastrophic failure that prompted ACCUMULO-2520 to verify that this change stops it?
src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
<https://reviews.apache.org/r/19546/#comment70150>
nit: would you mind including {} for the if block?
src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
<https://reviews.apache.org/r/19546/#comment70151>
nit: would you mind including {} for the if block?
src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java
<https://reviews.apache.org/r/19546/#comment70149>
Please remove empty javadoc blocks
Once this is forward ported to 1.6.0-SNAP, will it have to switch to absolute paths? both absolute and relative?
- Sean Busbey
On March 21, 2014, 7:27 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 7:27 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38231
-----------------------------------------------------------
Ship it!
Ship It!
- Sean Busbey
On March 21, 2014, 10:34 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 10:34 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java 722fd1a
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
> test/system/auto/simple/gc.py 4697246
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by ke...@deenlo.com.
> On March 21, 2014, 10:51 p.m., Sean Busbey wrote:
> > test/system/auto/simple/gc.py, lines 87-91
> > <https://reviews.apache.org/r/19546/diff/2/?file=532017#file532017line87>
> >
> > Is it possible for the GC to delete all the files in HDFS under ACCUMULO-2520, but still have this verification call pass? e.g. if all the table entries fit in memory?
at the beginning of the test test_ingest table is flushed
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38227
-----------------------------------------------------------
On March 21, 2014, 10:34 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 10:34 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java 722fd1a
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
> test/system/auto/simple/gc.py 4697246
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/#review38227
-----------------------------------------------------------
src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
<https://reviews.apache.org/r/19546/#comment70267>
Not an issue, but if we have lots of places that use functions like these, it might be worth a follow on ticket to switch to Guava's Verify class.
test/system/auto/simple/gc.py
<https://reviews.apache.org/r/19546/#comment70257>
Is it possible for the GC to delete all the files in HDFS under ACCUMULO-2520, but still have this verification call pass? e.g. if all the table entries fit in memory?
- Sean Busbey
On March 21, 2014, 10:34 p.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19546/
> -----------------------------------------------------------
>
> (Updated March 21, 2014, 10:34 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2520
> https://issues.apache.org/jira/browse/ACCUMULO-2520
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
>
>
> Diffs
> -----
>
> src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
> src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java 722fd1a
> src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
> src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
> test/system/auto/simple/gc.py 4697246
>
> Diff: https://reviews.apache.org/r/19546/diff/
>
>
> Testing
> -------
>
> Added unit test
> Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request 19546: ACCUMULO-2520 made GC validate data read from
!METADATA table
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19546/
-----------------------------------------------------------
(Updated March 21, 2014, 10:34 p.m.)
Review request for accumulo.
Changes
-------
Updated gc functional test to trigger this issue.
Removed empty comment and added {} for if stmt
Bugs: ACCUMULO-2520
https://issues.apache.org/jira/browse/ACCUMULO-2520
Repository: accumulo
Description
-------
This change should make the GC more robust against the case of it deleting things it should not. A concern with this change is that may make the GC fail in a situation where it should not, which could cause hdfs to fill.
Diffs (updated)
-----
src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 22c3c0e
src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java 722fd1a
src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java PRE-CREATION
src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java be444dd
test/system/auto/simple/gc.py 4697246
Diff: https://reviews.apache.org/r/19546/diff/
Testing
-------
Added unit test
Ran 1.4.5-SNAPSHOT and verified GC worked and deleted an orphaned file
Thanks,
kturner