Refactor "Corrupt Data" Tests in TestHLogSplit
Review Request #1115 - submitted 3 years, 5 months ago
| Alex Newman | Reviewers | ||
| hbase | |||
| hbase-2935 | nspiegelberg | ||
| None | hbase | ||
So I took the approach taken with the instrumented sequence file rider. Not sure if exposing those methods in the WALReader is cool so I am down with other suggestions. - Add a FaultySequenceFileLogReader - Un-commented the 2935 tests
Running on our internal hudson as I type this. Also I ran the TestHLogSplit test locally. I'll update the review if hudson is green.
Posted 3 years, 5 months ago (October 28th, 2010, 10:40 p.m.)
This looks great to me. Only one thing that seems odd, the first thing below. Nicolas, what you think?
-
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java (Diff revision 1) -
Do you need to do this? You can't get your WALReader in before first construction? I think I see why you want to use it... its because the test already has an HLog? What about just making a new HLog instance in your tests so you don't have to have this?
-
enum.valueOf might help here? see http://leepoint.net/notes-java/oop/enums/enums.html
Review request changed
Updated 3 years, 5 months ago (October 29th, 2010, 1:24 p.m.)
-
- added nspiegelberg
Posted 3 years, 5 months ago (October 29th, 2010, 2:39 p.m.)
-
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java (Diff revision 1) -
i had a quick look and i dont think there is a way out, since logWriterClass (and reader) are statics, there is no way to be first to the punch, since i believe surefire is running all the tests in 1 VM. Probably making this a package private instead woudl be a good goal. It'd be nice if there was a annotation to indicate this was test hook, but doc should suffice for now. so 'static void resetLogReaderClass'
Review request changed
Updated 3 years, 5 months ago (October 29th, 2010, 3:03 p.m.)
-
- added Diff r2
sta^ck and ryan's suggestions
Posted 3 years, 5 months ago (October 29th, 2010, 3:44 p.m.)
-
src/test/java/org/apache/hadoop/hbase/regionserver/wal/FaultySequenceFileLogReader.java (Diff revisions 1 - 2) -
woops pretend this says return FailureType.valueOf(conf.get("faultysequencefilelogreader.failuretype", FailureType.NONE.name()));
I'm okay with the general concept/direction. I think you should get rid of resetLogReaderClass(), but other than that, it looks pretty clean :) I think there's more edge cases with IOE/ParseExceptions that we could theoretically handle, but this is a positive step and mitigates the regression.
Other reviews