> On 2012-05-11 23:42:33, Francis Liu wrote: > > Looks good to me. Just a concern with the change in the test output > > directory. If you agree with any of my proposals, I can change that on > > commit. Will run e2e tests. > > Travis Crawford wrote: > Sure, either approach sounds fine. The main thing is being able to run in > the IDE. Do you want to make the change at commit time or for me to make it? > > Francis Liu wrote: > Cool. I don't mind either way. > > Travis Crawford wrote: > Hey Francis, > > What do you think about checking this in as-is, and we can address the > logs directory in https://issues.apache.org/jira/browse/HCATALOG-374 ? > There's a lot of inconsistent setting up of a test metastore and I'd like to > consolidate that setup code in a base test class so updates like these are > available to all tests. I posted a patch in that issue you can look at as an > example of what I'm proposing. > > Francis Liu wrote: > Hi Travis, if you're addressing this issue in a separate Jira. I'd prefer > not checking in this portion of the patch as it might affect the automated > builds. > > Travis Crawford wrote: > The tests pass locally so CI should not be affected. In general I'm not a > fan of builds producing things outside the build directory, because it makes > locating test data and logs more difficult, and the clean target needs to do > more than simply deleting the build directory, so there are opportunities for > clean to miss things. Putting files in directories with the test class name > ensures state does not get shared between tests. > > Please let me know what you'd like me to do here so we can wrap this up. > This patch has been going for over a month now and I'd really like to reach > closure and move on to new issues. > > Francis Liu wrote: > I see, I'm no fan either and since you're addressing it in a separate > jira. I'd rather we make the changes there rather than making it point to > potentially another external directory. I'll check this in without this > section. Let me know if you're ok with that.
That would be great. Thanks! - Travis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4971/#review7816 ----------------------------------------------------------- On 2012-05-05 01:23:50, Travis Crawford wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4971/ > ----------------------------------------------------------- > > (Updated 2012-05-05 01:23:50) > > > Review request for hcatalog and Francis Liu. > > > Summary > ------- > > Update ProgressReporter to work with both old and new mapreduce API. Delay > creating the base record reader so we have a StatusReporter and can use > counters. > > > This addresses bug HCATALOG-373. > https://issues.apache.org/jira/browse/HCATALOG-373 > > > Diffs > ----- > > src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java 268167e > src/java/org/apache/hcatalog/mapreduce/HCatRecordReader.java 65f96f4 > src/java/org/apache/hcatalog/mapreduce/InternalUtil.java 1837081 > src/java/org/apache/hcatalog/mapreduce/ProgressReporter.java fb379cd > src/test/org/apache/hcatalog/mapreduce/HCatMapReduceTest.java f3d07a0 > > Diff: https://reviews.apache.org/r/4971/diff > > > Testing > ------- > > "ant clean test" passes > > I can run pig+hcatalog queries using Elephant-Bird deprecated API wrappers, > which is why this issue originally came up. > > > Thanks, > > Travis > >
