You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Josh Elser <jo...@gmail.com> on 2014/10/01 01:01:40 UTC
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/
-----------------------------------------------------------
(Updated Sept. 30, 2014, 11:01 p.m.)
Review request for accumulo.
Changes
-------
Incorporated feedback from Eric
Bugs: ACCUMULO-3182
https://issues.apache.org/jira/browse/ACCUMULO-3182
Repository: accumulo
Description
-------
Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
Diffs (updated)
-----
server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/26153/diff/
Testing
-------
New IT, ran UTs and ITs.
Thanks,
Josh Elser
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 1, 2014, 12:48 a.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java, line 313
> > <https://reviews.apache.org/r/26153/diff/1-2/?file=708545#file708545line313>
> >
> > what about EOF check here? Why have multiple try/catch blocks for EOFE, why not one try/catch block that covers a lot of code?
Yeah, you're right. I totally missed this one. I was being obstinate against just throwing a big try/catch around it -- that would be much simpler in the long run.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55040
-----------------------------------------------------------
On Sept. 30, 2014, 11:01 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 11:01 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55040
-----------------------------------------------------------
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java
<https://reviews.apache.org/r/26153/#comment95371>
what about EOF check here? Why have multiple try/catch blocks for EOFE, why not one try/catch block that covers a lot of code?
- kturner
On Sept. 30, 2014, 11:01 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 11:01 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 1, 2014, 1:02 a.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java, line 196
> > <https://reviews.apache.org/r/26153/diff/3/?file=709899#file709899line196>
> >
> > Could use the following here...
> >
> > MetadataSchema.TabletsSection.getRow(tableId, null);
Ah, thanks! I missed that helpful method.
> On Oct. 1, 2014, 1:02 a.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java, line 216
> > <https://reviews.apache.org/r/26153/diff/3/?file=709899#file709899line216>
> >
> > this sleep should not be needed
I agree with you, but I still see https://issues.apache.org/jira/browse/ACCUMULO-2964 infrequently. Offline'ing the table should actually preclude the unexplained Thrift exception (if I'm right in that it's tied to the tserver restart) and it would actually make the test more generally usable.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55044
-----------------------------------------------------------
On Sept. 30, 2014, 11:01 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 11:01 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55044
-----------------------------------------------------------
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java
<https://reviews.apache.org/r/26153/#comment95374>
Could use the following here...
MetadataSchema.TabletsSection.getRow(tableId, null);
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java
<https://reviews.apache.org/r/26153/#comment95377>
I wonder if the test would work and run faster if the table was taken offline, metadata table modified, and then table brought back online. I have no problem w/ this approach, this was just something I thought of..
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java
<https://reviews.apache.org/r/26153/#comment95376>
this sleep should not be needed
- kturner
On Sept. 30, 2014, 11:01 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2014, 11:01 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 2, 2014, 3:49 p.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java, line 75
> > <https://reviews.apache.org/r/26153/diff/3-4/?file=709899#file709899line75>
> >
> > This is funny. Was this something you only saw after switching to offline/online? I suppose wacking the tserver kept gc from running.
Haha yup. Every ~1/10 runs of the test case, I'd get a FileNotFoundException when the tserver would try to read the file. Best I could guess is that the GC cleaned up the file because it looked like a WAL and wasn't referenced at all in metadata. :)
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55209
-----------------------------------------------------------
On Oct. 1, 2014, 7:15 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 7:15 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/#review55209
-----------------------------------------------------------
Ship it!
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java
<https://reviews.apache.org/r/26153/#comment95599>
This is funny. Was this something you only saw after switching to offline/online? I suppose wacking the tserver kept gc from running.
- kturner
On Oct. 1, 2014, 7:15 p.m., Josh Elser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26153/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 7:15 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3182
> https://issues.apache.org/jira/browse/ACCUMULO-3182
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
>
>
> Diffs
> -----
>
> server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
> server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
> server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
> server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
> server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
> server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
> test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26153/diff/
>
>
> Testing
> -------
>
> New IT, ran UTs and ITs.
>
>
> Thanks,
>
> Josh Elser
>
>
Re: Review Request 26153: ACCUMULO-3182 WAL recovery on empty files
doesn't fail
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26153/
-----------------------------------------------------------
(Updated Oct. 1, 2014, 7:15 p.m.)
Review request for accumulo.
Changes
-------
* Lifted the try/catch(EOFException) around main body of DfsLogger.readHeadeAndReturnStream
* Fixed an NPE in LogSorter.close when we get one of the WALs
* Used offline/online to trigger recovery on the table
* Used more MetadataSchema helper methods
* Added a delay to the GC start as it would sometimes remove the file we created before the metadata reference was added
Bugs: ACCUMULO-3182
https://issues.apache.org/jira/browse/ACCUMULO-3182
Repository: accumulo
Description
-------
Ensures that a WAL that was empty or missing a complete header (due to tservers dying before it could happen) can be successfully recovered (by assuming that the lack of a complete header means there is no data to recover). Currently against master, will pull back to 1.5 and merge forward since it likely affects all branches.
Diffs (updated)
-----
server/base/src/main/java/org/apache/accumulo/server/log/SortedLogState.java PRE-CREATION
server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java 791aec8
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b4fbfed
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 8de2b25
server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java 4badefa
server/tserver/src/main/java/org/apache/accumulo/tserver/log/MultiReader.java 541f075
server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogReader.java 681fbd3
server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java 416a86e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/MultiReaderTest.java a79e77e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java fffa15e
server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java f01ee10
test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/26153/diff/
Testing
-------
New IT, ran UTs and ITs.
Thanks,
Josh Elser