----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7384/#review12104 -----------------------------------------------------------
Thanks for taking a first pass at this. I stopped partway though the review since there were some patterns emerging, rather than list everything explicitly. * For private methods, I believe we can just change the signature. * Some methods are public but really intended for internal use. For such methods, I think we can annotate them with InterfaceAudience/InterfaceStability and change the signature. * Some public methods could be made private, such as HCatBaseInputFormat.getOutputSchema Can you also add a bit of context to the Jira about why this change would be useful. We discussed over IM but others are probably scratching their heads over why you wouldn't have a Job and instead want to pass these parameters around yourself. src/java/org/apache/hcatalog/common/HCatUtil.java <https://reviews.apache.org/r/7384/#comment25784> I believe we can change this interface, and not provide the backwards-compatible signature. The only usages I found of this method are: * HCatBaseOutputFormat.configureOutputStorageHandler() * HCatOutputFormat.setOutput() And I don't see why this method would need to be exposed to other tools. Instead of the backwards-compatible method, how about removing it and annotating this method with: @InterfaceAudience.Private @InterfaceStability.Evolving That way we can still have it public for internal use, but signal to others not to directly use it. src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/7384/#comment25785> "Find usages" in my IDE showed this is only called from: HCatBaseInputFormat.createRecordReader() If that's true perhaps we could make this private and change the signature. src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/7384/#comment25786> This is another example of a public method that might be a bit permissive. The only main codepath usage I found is: HCatBaseInputFormat.getOutputSchema() However, its called quite a few times from tests. Perhaps we should annotate this as: @InterfaceAudience.Private @InterfaceStability.Evolving and change the signature. Somewhat related note: when adding these wrapper classes, I find javadocs get out-of-date when copied like this. Perhaps use @see and have the description in just one place? src/java/org/apache/hcatalog/mapreduce/HCatBaseInputFormat.java <https://reviews.apache.org/r/7384/#comment25788> This is private so I believe we can change the signature. - Travis Crawford On Oct. 1, 2012, 11:34 p.m., Nitay Joffe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7384/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2012, 11:34 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 > ce6562383e55ff80c75731da3ba780373e58c9a6 > src/java/org/apache/hcatalog/mapreduce/Security.java > 041a89845f78d5773edac5a50eeab1a15993e528 > > Diff: https://reviews.apache.org/r/7384/diff/ > > > Testing > ------- > > > Thanks, > > Nitay Joffe > >
