giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Avery Ching" <avery.ch...@gmail.com>
Subject Re: Review Request 12322: GIRAPH-709: More flexible Jython script loading
Date Tue, 09 Jul 2013 19:28:31 GMT


> On July 9, 2013, 6:54 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java, line
89
> > <https://reviews.apache.org/r/12322/diff/2/?file=319001#file319001line89>
> >
> >     Isn't this a real error?
> 
> Nitay Joffe wrote:
>     Yes you're right I'll pass the IOException up.

Or your can IllegalStateException it.


> On July 9, 2013, 6:54 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java, lines
135-145
> > <https://reviews.apache.org/r/12322/diff/2/?file=319001#file319001line135>
> >
> >     Usually
> >     @Override is on the previous line.
> 
> Nitay Joffe wrote:
>     Yeah my intellij keeps generating them this way, will fix.

Thanks.


> On July 9, 2013, 6:54 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java, lines 198-199
> > <https://reviews.apache.org/r/12322/diff/2/?file=319002#file319002line198>
> >
> >     Not a big fan of Jython specific stuff here.  Can it be done in the JythonComputationFactory
perchance?
> 
> Nitay Joffe wrote:
>     It *could*, but that assumes you are using a JythonComputationFactory, and I'd like
to separate those two things. Part of the point of this diff is to separate jython scripts
from just the computation factory (it was tied before). Soon we will have Aggregators, MasterCompute,
and so on all possibly in Jython, and the user will naturally want to organize their Jython
code. Also, potentially you could have a bunch of things in Jython but actually implement
the Computation itself in Java. Also, when we support other languages (JRuby, Groovy, etc)
this JythonLoader could easily become a general "script loader" that gets called at start
of each node to make sure things have been processed. Make sense, what do you think?

I think that having a generic script loader seems fairly reasonable.  Can you change it that
way?


> On July 9, 2013, 6:54 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/jython/JythonLoader.java, lines 69-70
> > <https://reviews.apache.org/r/12322/diff/2/?file=319005#file319005line69>
> >
> >     I don't see where these are called.  Why would we want to do this?  Would it
be in the case of say one for computation, one for combiner, etc?
> 
> Nitay Joffe wrote:
>     Yes exactly, supporting any number of jython scripts so user could split things up
/ modularize.

Or generic scripts now, right?  


- Avery


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12322/#review22872
-----------------------------------------------------------


On July 9, 2013, 5 p.m., Nitay Joffe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12322/
> -----------------------------------------------------------
> 
> (Updated July 9, 2013, 5 p.m.)
> 
> 
> Review request for giraph.
> 
> 
> Bugs: GIRAPH-709
>     https://issues.apache.org/jira/browse/GIRAPH-709
> 
> 
> Repository: giraph-git
> 
> 
> Description
> -------
> 
> See JIRA.
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/benchmark/PageRankBenchmark.java 413107dd70ba48ab7647127481ff47039fd1a758

>   giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java e7af82565f38fbaafd06b4579acd4dce48e6f027

>   giraph-core/src/main/java/org/apache/giraph/jython/DeployedScript.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/jython/JythonComputationFactory.java f7331acc99909e665fae869a5fdb31d82b0a6e5c

>   giraph-core/src/main/java/org/apache/giraph/jython/JythonLoader.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/jython/JythonUtils.java 77040e309e873fbc3394ae144176f10b95e85faa

>   giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java aba51318b052693440ccb6131ae7bb3120bdac96

>   giraph-core/src/test/java/org/apache/giraph/jython/TestJython.java 58f25a67cdcf705f9cd845256ac6ebce8b4cc779

> 
> Diff: https://reviews.apache.org/r/12322/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Nitay Joffe
> 
>


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