> On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/it/java/org/apache/phoenix/mapreduce/CsvBulkLoadToolIT.java, > > line 76 > > <https://reviews.apache.org/r/19257/diff/1/?file=520485#file520485line76> > > > > 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)
This is a file being created on HDFS (not local FS), so couldn't platform-agnostic separators cause problems here? Closing it in a finally sounds like a good plan -- that being said, again this is on a transient HDFS cluster within the test, so there is no garbage that is going to be left behind after the test stops no matter what. > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java, > > line 92 > > <https://reviews.apache.org/r/19257/diff/1/?file=520490#file520490line92> > > > > Could this be a private method? No, it's used by the unit tests, so it needs to be package private. It should be annotated with @VisibleForTesting though. > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java, > > line 182 > > <https://reviews.apache.org/r/19257/diff/1/?file=520490#file520490line182> > > > > Probably using "java.io.tmpdir" is better. This is a path on HDFS, so java.io.tmpdir will point to something else. The explanation on the approach for the creating of this tempdir is in a comment above. > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java, > > line 231 > > <https://reviews.apache.org/r/19257/diff/1/?file=520490#file520490line231> > > > > private? Again, used by unit tests, but should be annotated with @VisibleForTesting. > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java, > > line 248 > > <https://reviews.apache.org/r/19257/diff/1/?file=520490#file520490line248> > > > > private method? Yep, this one really should be private :-) > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvBulkLoadTool.java, > > line 339 > > <https://reviews.apache.org/r/19257/diff/1/?file=520490#file520490line339> > > > > Should this be done in a finally and the exception thrown back anyway? Yes, nice catch on that. > On March 17, 2014, 12:50 a.m., Prashant Kommireddi wrote: > > phoenix-core/src/main/java/org/apache/phoenix/mapreduce/CsvToKeyValueMapper.java, > > line 176 > > <https://reviews.apache.org/r/19257/diff/1/?file=520491#file520491line176> > > > > Do you think we should catch a specific exception here? Yes, good point -- I think I had a specific reason for doing it this way, but I don't remember what the exact reason was and unfortunately I didn't document it here. Should be double-checked. - Gabriel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19257/#review37328 ----------------------------------------------------------- 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 > >
