----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7384/#review12441 -----------------------------------------------------------
Overall looks good, and I'll run tests now and kick the tires a bit. I gave the changes a bit more thought, in particular around making some stuff private, and how to deal with these compatibility methods. For the ones listed below, I think we should mark the "old" versions as deprecated so we can remove them in >= 0.6.0, so we're signaling to people to move over to using a Configuration when possible (which I think is a good thing in general instead of always using the Job). Also, thinking about making getTableSchema private, I think someone could reasonably want that for the IF (its already used in the OF) so we should keep it public. src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/7384/#comment26406> Looking at the two getTableSchema methods, one is public & one is private, so I've been giving some thought to what this should actually be. I checked the corresponding HCIF methods to see how they're used too. At this time the IF methods are used internally, but the OF methods are used by both the data transfer & pig adapter. Even though it doesn't currently need to be public, after some more thought I think we should keep getTableSchema public since its a reasonable thing for someone to get, like if someone wanted to get the input table schema to validate its usable by a framework adapter. Sorry to flip-flop on this one. Can you: * make getTableSchema public in both places? * annotate "getTableSchema(JobContext context)" as deprecated, with a note to remove in 0.6.0? * remove InterfaceAudience/InterfaceStability annotations. Since the method is a regular public one I don't think we need to treat it specially. src/java/org/apache/hcatalog/mapreduce/HCatBaseOutputFormat.java <https://reviews.apache.org/r/7384/#comment26412> Similar to the long comment in the input format, can you add a deprecated annotation and mark for removal in 0.6.0? src/java/org/apache/hcatalog/mapreduce/HCatBaseOutputFormat.java <https://reviews.apache.org/r/7384/#comment26413> Let's mark as deprecated and will be removed in 0.6.0. - Travis Crawford On Oct. 9, 2012, 10:47 p.m., Nitay Joffe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7384/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2012, 10:47 p.m.) > > > Review request for hcatalog. > > > Description > ------- > > HCATALOG-516: HCOF refactor to allow calling without Job > > > Diffs > ----- > > src/java/org/apache/hcatalog/common/HCatUtil.java > 10446e163a3671799bf4eb16b5a112bf4d7cd1e3 > src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java > 3532696410ae9da17a815c03aa35d85ffc446152 > src/java/org/apache/hcatalog/mapreduce/HCatBaseOutputFormat.java > d741b7fbd1bb8bc3da19064cfc1e733267ee9ee7 > src/java/org/apache/hcatalog/mapreduce/HCatInputFormat.java > b668d7ad8cbe45602b668b560cd39d465151785a > src/java/org/apache/hcatalog/mapreduce/HCatOutputFormat.java > 30c9e6bce0181b21e01356eedebaa2781f06b8a3 > src/java/org/apache/hcatalog/mapreduce/InitializeInput.java > df5afad1fb6ed0b7a4ce3b5cf13a620fecb9bcb1 > src/java/org/apache/hcatalog/mapreduce/Security.java > 041a89845f78d5773edac5a50eeab1a15993e528 > > Diff: https://reviews.apache.org/r/7384/diff/ > > > Testing > ------- > > > Thanks, > > Nitay Joffe > >
