giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nitay Joffe" <ni...@apache.org>
Subject Re: Review Request: Refactor CLI arg parsing out of GiraphRunner
Date Thu, 07 Feb 2013 04:14:13 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message