> On May 16, 2014, 8:19 p.m., Josh Elser wrote: > > server/tserver/src/test/java/org/apache/accumulo/tserver/log/LocalWALRecoveryTest.java, > > line 75 > > <https://reviews.apache.org/r/21567/diff/1/?file=583640#file583640line75> > > > > Wouldn't it have simplified things to just use the RawLocalFileSystem > > over the ChecksumFileSystem? I'm not sure if there are any drawbacks to > > using the RawLocalFileSystem off the top of my head. > > Sean Busbey wrote: > Maybe? > > I don't see an obvious place where we're choosing one or the other. If > hte ChecksumFileSystem is the default when giving a Hadoop configuration that > doesn't use HDFS, I'd rather stay at defaults. That way if we want later we > can inject different configurations to do minidfs or an actual cluster or > whatever. > > Josh Elser wrote: > Hadoop defaults to the ChecksumFileSystem for the fs.file.impl (or > whatever the non-deprecated property is these days). Because we're not > writing, it's probably not a big deal (the notable difference I'm aware of is > that ChecksumFileSystem doesn't implement flush/sync semantics properly). I'm > not sure what the old loggers did in 1.4 -- you or Mike probably know better > than I do (did they even use the Hadoop FileSystem API?). > > I'm not sure I understand what you meant about injecting configurations > to minidfs because this is purely for reading data off of local filesystem. > > Sean Busbey wrote: > If it's just for the reading portion and not when we write the recovery > files, won't we see the crc file when using the raw version? > > Josh Elser wrote: > The point is that you can *completely remove* the CRC handling code > that's in the test case which is completely useless. But given that you > dropped this already, I'm assuming you don't care.
updated review uses the FileSystem api instead of File, so there's no checking for crc files. That work acceptably? - Sean ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21567/#review43255 ----------------------------------------------------------- On May 16, 2014, 11:29 p.m., Sean Busbey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21567/ > ----------------------------------------------------------- > > (Updated May 16, 2014, 11:29 p.m.) > > > Review request for accumulo, Josh Elser and Mike Drob. > > > Bugs: ACCUMULO-2816 > https://issues.apache.org/jira/browse/ACCUMULO-2816 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2816 Ensure LocalWALRecovery looks at WAL. > > * consolidated filtering of crc so that underlying fs return order on > directory contents doesn't matter > * added better error message for reading an invalid wal > * fixed v2 vs v3 magic header reading > > > Diffs > ----- > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java > eb04f09021f313c2c12ad15f2ae443c368452418 > > server/tserver/src/main/java/org/apache/accumulo/tserver/logger/LogFileKey.java > 1e33569af680079b72dde6e57833b096f5310129 > > server/tserver/src/test/java/org/apache/accumulo/tserver/log/LocalWALRecoveryTest.java > 99190b2a3ae6773f70ce4b5508f1b3dab19cfd3e > > Diff: https://reviews.apache.org/r/21567/diff/ > > > Testing > ------- > > unit tests pass (they did not on my system prior to this patch) > > > Thanks, > > Sean Busbey > >
