----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9350/#review16249 -----------------------------------------------------------
This is great, I love it. A few notes inline but I fully support this direction. giraph-core/src/main/java/org/apache/giraph/GiraphRunner.java <https://reviews.apache.org/r/9350/#comment34720> We have another runner for Hive jobs, called HiveGiraphRunner. You should take a look at that as well. At Facebook we also have our own Runner that inherits from HiveGiraphRunner. To be honest both of these classes (ours and HiveGiraphRunner) need to go away in my opinion. Instead we should be able to just plugin additional options parsers, so that if you want Hive you plugin some HiveOptionsParser, we'll have our CompanyOptionsParser... but everyone will use the same one modular GiraphRunner. Does that make sense? Anyways this can be in a separate issue but wanted to get you thinking about it. giraph-core/src/main/java/org/apache/giraph/GiraphRunner.java <https://reviews.apache.org/r/9350/#comment34712> nit: EdgeInputFormat (I realize not your code, but might as well fix) giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34713> Let's just make an Enum and return it rather than these ints with meaning. giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34716> I don't think you want this here..? giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34715> I don't think you want this here..? giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34714> I don't think you want this here..? giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34718> I don't think you want this here..? giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34717> I don't think you want this here..? giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java <https://reviews.apache.org/r/9350/#comment34719> We have similar-ish code in HiveGiraphRunner -hiveconf parameter, can we consolidate somehow? (possibly separate task) - Nitay Joffe On Feb. 7, 2013, 3 a.m., Eli Reisman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9350/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2013, 3 a.m.) > > > Review request for giraph. > > > Description > ------- > > Refactor CLI parsing of args and the assembly of the GiraphConfiguration into > its own class so that other future Runner classes besides GiraphRunner can > use it w/o duplication. You get the idea. > > > Diffs > ----- > > giraph-core/src/main/java/org/apache/giraph/GiraphRunner.java b6a6113 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java > fb4e8a3 > giraph-core/src/main/java/org/apache/giraph/utils/CliParserUtils.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9350/diff/ > > > Testing > ------- > > > Thanks, > > Eli Reisman > >
