> 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 > > Travis Crawford wrote: > 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.
I am +1 on the following APIs: HCatInputFormat.setInput(job, dbName, inputTableName).setFilter(filter).setProperties(props); HCatInputFormat.setInput(conf, dbName, inputTableName).setFilter(filter).setProperties(props); - Vandana ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8148/#review13694 ----------------------------------------------------------- On Nov. 29, 2012, 1:46 a.m., Travis Crawford wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8148/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2012, 1:46 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/data/TestReaderWriter.java 18d368f > 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 > 5a4d400 > > 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 > >
