----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19257/#review37328 -----------------------------------------------------------
bin/readme.txt <https://reviews.apache.org/r/19257/#comment68828> Is this always a temporary position? phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java <https://reviews.apache.org/r/19257/#comment68829> Should this be named *Test, say CsvBulkLoadToolTest ? phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java <https://reviews.apache.org/r/19257/#comment68830> You might want to do this in a try-catch and do closing of stream/writer in a finally. Also, creating the tmp path would be better if its platform agnostic. See http://docs.oracle.com/javase/7/docs/api/java/io/File.html#createTempFile(java.lang.String, java.lang.String) phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java <https://reviews.apache.org/r/19257/#comment68831> Same here, this should probably be done in a finally. phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java <https://reviews.apache.org/r/19257/#comment68832> Same as above phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkImportUtil.java <https://reviews.apache.org/r/19257/#comment68833> You could have a private constructor if it's only static methods on this. phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java <https://reviews.apache.org/r/19257/#comment68834> Could this be a private method? phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java <https://reviews.apache.org/r/19257/#comment68835> Probably using "java.io.tmpdir" is better. phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java <https://reviews.apache.org/r/19257/#comment68836> private? phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java <https://reviews.apache.org/r/19257/#comment68837> private method? phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java <https://reviews.apache.org/r/19257/#comment68838> Should this be done in a finally and the exception thrown back anyway? phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvToKeyValueMapper.java <https://reviews.apache.org/r/19257/#comment68839> Do you think we should catch a specific exception here? - Prashant Kommireddi On March 15, 2014, 9:16 p.m., Gabriel Reid wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19257/ > ----------------------------------------------------------- > > (Updated March 15, 2014, 9:16 p.m.) > > > Review request for phoenix. > > > Repository: phoenix > > > Description > ------- > > Rewrite of the Phoenix MapReduce import, to follow more standard MR tool > development patterns and use standard Phoenix CSV handling functionality. > > > Diffs > ----- > > bin/csv-bulk-loader.py 385ef41 > bin/readme.txt fa23eeb > phoenix-assembly/pom.xml 9a69cab > phoenix-assembly/src/build/all.xml 9c1bc41 > phoenix-assembly/src/build/mapreduce.xml PRE-CREATION > > phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java > PRE-CREATION > phoenix-core/src/main/java/org/apache/phoenix/map/reduce/CSVBulkLoader.java > 0fd74e1 > phoenix-core/src/main/java/org/apache/phoenix/map/reduce/MapReduceJob.java > 9dc8032 > > phoenix-core/src/main/java/org/apache/phoenix/map/reduce/util/ConfigReader.java > 1c94d6f > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkImportUtil.java > PRE-CREATION > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java > PRE-CREATION > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvToKeyValueMapper.java > PRE-CREATION > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ImportPreUpsertKeyValueProcessor.java > PRE-CREATION > phoenix-core/src/main/java/org/apache/phoenix/schema/PDataType.java 34166d3 > phoenix-core/src/main/java/org/apache/phoenix/util/CSVCommonsLoader.java > 3e86477 > phoenix-core/src/main/java/org/apache/phoenix/util/ColumnInfo.java 37238c8 > > phoenix-core/src/test/java/org/apache/phoenix/mapreduce/CsvBulkImportUtilTest.java > PRE-CREATION > > phoenix-core/src/test/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolTest.java > PRE-CREATION > > phoenix-core/src/test/java/org/apache/phoenix/mapreduce/CsvToKeyValueMapperTest.java > PRE-CREATION > phoenix-core/src/test/java/org/apache/phoenix/util/ColumnInfoTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/19257/diff/ > > > Testing > ------- > > > Thanks, > > Gabriel Reid > >
