> On Nov. 22, 2012, 7:29 p.m., Daniel Dai wrote: > > Here is my thinking: > > >> * its cumbersome for users to say job.getConfiguration instead of > > >> passing in a job - should we provide that convenience? > > I think not, actually it provides more flexibility cuz sometimes user only > > get configuration not job. > > >> * filter is optional - should we provide a method that automatically > > >> nulls out? > > Sure, why not > > >> * properties is optional and rarely used (I believe only in the hbase > > >> storage handler) - should we provide a method that automatically sets to > > >> null? > > Sure, why not
Hey Daniel & Vandana, thanks for taking a look. I'm getting mixed feedback on Configuration/Job as a parameter. I sort of agree job.getConfiguration() is something users will have to do often, and we should provide the convenience of passing in either one. The javadoc can be clear about being a convenience wrapper around the conf version. This means we'd need the following: HCatInputFormat.setInput(job, dbName, inputTableName); HCatInputFormat.setInput(job, dbName, inputTableName, filter); HCatInputFormat.setInput(job, dbName, inputTableName, filter, properties); HCatInputFormat.setInput(conf, dbName, inputTableName); HCatInputFormat.setInput(conf, dbName, inputTableName, filter); HCatInputFormat.setInput(conf, dbName, inputTableName, filter, properties); All would be wrappers around the last signature. This seems annoying for users. Giving this more thought, what do y'all think about: HCatInputFormat.setInput(job, dbName, inputTableName).setFilter(filter).setProperties(props); HCatInputFormat.setInput(conf, dbName, inputTableName).setFilter(filter).setProperties(props); I think this would be easier for users, as well as being more flexible over time if we need to add optional parameters. Thoughts? Let me know which approach you think would be better for users and I can update the patch. - Travis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8148/#review13694 ----------------------------------------------------------- On Nov. 21, 2012, 1:01 a.m., Travis Crawford wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8148/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2012, 1:01 a.m.) > > > Review request for hcatalog. > > > Description > ------- > > Currently we expose the following interface to use HCatInputFormat: > > """ > HCatInputFormat.setInput(job, InputJobInfo.create(dbName, inputTableName, > null)); > """ > > Notice how InputJobInfo is exposed directly to end-users. InputJobInfo should > be treated as an implementation detail, and the current API should be > deprecated and later removed. > > This change is my first pass at deprecating the above. Feedback would be > appreciated. In particular, I'm interested in what method signatures we > should provide people. At present we need the following: > > Configuration - required > database name - required > table name - required > filter - optional > properties - optional > > To keep things simple I depercated everything except a single constructor > that takes those above arguments. > > Questions: > * its cumbersome for users to say job.getConfiguration instead of passing in > a job - should we provide that convenience? > * filter is optional - should we provide a method that automatically nulls > out? > * properties is optional and rarely used (I believe only in the hbase storage > handler) - should we provide a method that automatically sets to null? > > > This addresses bug HCATALOG-527. > https://issues.apache.org/jira/browse/HCATALOG-527 > > > Diffs > ----- > > > core/src/main/java/org/apache/hcatalog/data/transfer/impl/HCatInputFormatReader.java > cf9bba2 > core/src/main/java/org/apache/hcatalog/mapreduce/HCatInputFormat.java > 7a7446f > core/src/main/java/org/apache/hcatalog/mapreduce/InitializeInput.java > e91ddc7 > core/src/main/java/org/apache/hcatalog/mapreduce/InputJobInfo.java 8779c5e > core/src/test/java/org/apache/hcatalog/mapreduce/HCatMapReduceTest.java > 74c4888 > core/src/test/java/org/apache/hcatalog/mapreduce/TestHCatInputFormat.java > a6b381e > hcatalog-pig-adapter/src/main/java/org/apache/hcatalog/pig/HCatLoader.java > 4bdb7c3 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseBulkOutputFormat.java > 3875185 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseDirectOutputFormat.java > 81a3099 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestHBaseInputFormat.java > 78724a1 > > storage-handlers/hbase/src/test/org/apache/hcatalog/hbase/TestSnapshots.java > e07bf46 > > Diff: https://reviews.apache.org/r/8148/diff/ > > > Testing > ------- > > I have not run tests yet as I'm soliciting feedback on the interface change > at this time. Will run tests and our internal integration tests before > posting a final version. > > > Thanks, > > Travis Crawford > >
