----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1184/#review1167 -----------------------------------------------------------
trunk/build.xml <https://reviews.apache.org/r/1184/#comment2411> Instead of checking it in lib, is it possible to pull this lib through ivy ? trunk/src/java/org/apache/hcatalog/common/HCatConstants.java <https://reviews.apache.org/r/1184/#comment2412> Do we need these keys? Looks like you are propagating the token already acquired in the first job to the second one. trunk/src/java/org/apache/hcatalog/common/HCatConstants.java <https://reviews.apache.org/r/1184/#comment2413> Will it be better to externalize this property? Otherwise one has to recompile hcatalog turn this off. trunk/src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/1184/#comment2414> Do we need all these methods ? Looks like you no longer need not to acquire token from JT. trunk/src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/1184/#comment2415> Looks little unnatural to do logging this way. We should take advantage of the logging library to accomplish these tasks. trunk/src/java/org/apache/hcatalog/har/HarOutputCommitterPostProcessor.java <https://reviews.apache.org/r/1184/#comment2416> Commented code. Either remove it or uncomment it. I think these logging statements should be enabled. trunk/src/java/org/apache/hcatalog/har/HarOutputCommitterPostProcessor.java <https://reviews.apache.org/r/1184/#comment2417> Commented code. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2418> Looks like you are not using this map later on. We should probably remove it. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2419> This I think is because you don't have a list of base committers initially. But it may be possible to get that list from HCatRecordWriter at this point and then call commitTask() on them. Calling commitTask from close() looks awkward, if there is a way we should avoid that. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2420> Instead of special casing for dynamic partitioning we should always call baseComitter.setupTask() here. In case of dynamic partitions, this will become noop from here and latter call it again from write() to do the real work. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2421> Can you explain the purpose of this boolean? Looks like it can be derived from existing variables. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2422> Looks like this isn't needed anymore. trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java <https://reviews.apache.org/r/1184/#comment2423> Why we depend on this variable? Shouldn't we always do fs.exists() check? trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitterPostProcessor.java <https://reviews.apache.org/r/1184/#comment2424> I wonder if we are too early in introducing this interface. There is only one impl which is always called (in case of dynamic partitions). I think we should introduce interfaces only if there is a known use case for it. Further when other use cases do come up, good likelihood that we need to tweak the interface to accomodate them. trunk/src/java/org/apache/hcatalog/pig/HCatStorer.java <https://reviews.apache.org/r/1184/#comment2425> Looks like some of these are not required. trunk/src/java/org/apache/hcatalog/pig/PigHCatUtil.java <https://reviews.apache.org/r/1184/#comment2426> retrieve.. do you want to rename it to getFromUDF.. trunk/src/test/org/apache/hcatalog/pig/TestHCatStorer.java <https://reviews.apache.org/r/1184/#comment2427> In all of these test cases, you should also assert metadata, by retrieving them from metastore and verifying as you are expecting them. - Ashutosh On 2011-07-22 16:09:41, Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1184/ > ----------------------------------------------------------- > > (Updated 2011-07-22 16:09:41) > > > Review request for hcatalog. > > > Summary > ------- > > HCatalog-42 review request on behalf of Sushanth > > > This addresses bug HCATALOG-42. > https://issues.apache.org/jira/browse/HCATALOG-42 > > > Diffs > ----- > > trunk/build.xml 1149353 > trunk/src/java/org/apache/hcatalog/common/ErrorType.java 1149353 > trunk/src/java/org/apache/hcatalog/common/HCatConstants.java 1149353 > trunk/src/java/org/apache/hcatalog/common/HCatUtil.java 1149353 > trunk/src/java/org/apache/hcatalog/har/HarOutputCommitterPostProcessor.java > PRE-CREATION > trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseOutputCommitter.java > 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatBaseOutputFormat.java > 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatEximOutputCommitter.java > 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatEximOutputFormat.java > 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitter.java > 1149353 > > trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputCommitterPostProcessor.java > PRE-CREATION > trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputFormat.java 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatOutputStorageDriver.java > 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatRecordWriter.java 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/HCatTableInfo.java 1149353 > trunk/src/java/org/apache/hcatalog/mapreduce/OutputJobInfo.java 1149353 > trunk/src/java/org/apache/hcatalog/pig/HCatEximStorer.java 1149353 > trunk/src/java/org/apache/hcatalog/pig/HCatStorer.java 1149353 > trunk/src/java/org/apache/hcatalog/pig/PigHCatUtil.java 1149353 > trunk/src/java/org/apache/hcatalog/rcfile/RCFileMapReduceOutputFormat.java > 1149353 > trunk/src/java/org/apache/hcatalog/rcfile/RCFileOutputDriver.java 1149353 > trunk/src/test/org/apache/hcatalog/mapreduce/HCatMapReduceTest.java 1149353 > > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatDynamicPartitioned.java > PRE-CREATION > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatEximInputFormat.java > 1149353 > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatEximOutputFormat.java > 1149353 > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatNonPartitioned.java > 1149353 > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatOutputFormat.java > 1149353 > trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatPartitioned.java > 1149353 > trunk/src/test/org/apache/hcatalog/pig/TestHCatStorer.java 1149353 > > Diff: https://reviews.apache.org/r/1184/diff > > > Testing > ------- > > Unit tests are included > > > Thanks, > > Ashutosh > >
