> 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) > > Gabriel Reid wrote: > 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. > > Prashant Kommireddi wrote: > I'm sorry, you are right this is writing to HDFS. Ignore my comment > regarding using File API. > > Regarding closing streams, I'm less worried about the garbage and more > about closing streams cleanly. Would the streams be released/closed at the > end of tests regardless?
The open streams are to a HDFS/HBase cluster that is running only for the duration of the test, so I believe they'll be closed regardless at the end of the test (when the cluster gets destroyed). I'm often a bit torn about situations like this -- on the one hand it feels wrong to not ensure that things like this are cleanly closed, but on the other hand it can make the test a lot less readable if all the closing is done *really* correctly. - 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 > >
