----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6684/#review10489 -----------------------------------------------------------
Will, Thanks for the patch. Generally looks good. I have only one major concern - regarding the hadoop classpath. flume-ng-tests/pom.xml <https://reviews.apache.org/r/6684/#comment22391> A guava dependency has been introduced by using ImmutableList. It needs to be added to the pom flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestFileChannel.java <https://reviews.apache.org/r/6684/#comment22392> Nit: you could move all this initialization code to a setUp method so it can be used for other tests when we add them flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestFileChannel.java <https://reviews.apache.org/r/6684/#comment22390> There is a potential issue here. Dependencies of hadoop do not get added to the classpath. If the hadoop classes we use depend on anything which flume has not already pulled in, this would cause issues. I don't immediately have access to a machine with no hadoop, but if you do please see if this will run fine on that. Please confirm that this works on a machine without hadoop installed(hadoop binary is not available). flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestFileChannel.java <https://reviews.apache.org/r/6684/#comment22389> I'm afraid this will cause the same problem we were facing with the integration tests earlier, since it was possible that the test would fail because the agent didn't start up in 3 seconds. I don't mind this getting committed as is, but please file a followup jira. flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestFileChannel.java <https://reviews.apache.org/r/6684/#comment22388> Nit: Why not just read the actual output into memory and then compare the two strings. What is done here seems much more expensive - generate expected out, write it out, and then read both files back in to compare. - Hari Shreedharan On Aug. 18, 2012, 12:30 a.m., Will McQueen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6684/ > ----------------------------------------------------------- > > (Updated Aug. 18, 2012, 12:30 a.m.) > > > Review request for Flume. > > > Description > ------- > > First integration test for file channel > > > This addresses bug FLUME-1492. > https://issues.apache.org/jira/browse/FLUME-1492 > > > Diffs > ----- > > flume-ng-tests/pom.xml 7d5ebed > > flume-ng-tests/src/test/java/org/apache/flume/test/agent/TestFileChannel.java > e69de29 > flume-ng-tests/src/test/java/org/apache/flume/test/util/StagedInstall.java > 3e7940d > > Diff: https://reviews.apache.org/r/6684/diff/ > > > Testing > ------- > > > Thanks, > > Will McQueen > >
