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: Jython for Giraph
Date Tue, 11 Jun 2013 05:33:24 GMT


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/AbstractConfOption.java, lines
49-66
> > <https://reviews.apache.org/r/11709/diff/2/?file=302654#file302654line49>
> >
> >     my personal preference here is for adding boolean exists(Configuration conf),
rather than these two methods.  Matchs the Map interface

Where is exists, do you mean contains? Anyways I've changed it to exists let me know if you
want something else.


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/BooleanConfOption.java, line 49
> > <https://reviews.apache.org/r/11709/diff/2/?file=302655#file302655line49>
> >
> >     Should this be abstract in abstract conf option?

good call


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/EnumConfOption.java, lines 61-62
> > <https://reviews.apache.org/r/11709/diff/2/?file=302658#file302658line61>
> >
> >     Style-wise, we typically have a line between the comments and the parameters.
 I don't know how to enforce this in checkstyle, but can we preserve it?  Btw, you do this
everywhere...

I'll look through and fix where I see it.


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java, lines 526-533
> > <https://reviews.apache.org/r/11709/diff/2/?file=302660#file302660line526>
> >
> >     Is this acceptable, should we expect a null argument?

It's for all the updateSuperstep() stuff. I haven't looked through that code deeply but from
my understanding it sets the Computation class every superstep so that it can be configurable.
However in the case of Jython the Computation class is null, so it keeps passing null. I will
look at how to stop it upstream.

@maja you have any thoughts here I believe you added the superstep update stuff?


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java, lines 183-185
> > <https://reviews.apache.org/r/11709/diff/2/?file=302672#file302672line183>
> >
> >     Why do we expect this to be set sometimes and not others?

I think you're right that shouldn't happen, but I'll double check. It's a bit tricky... The
current logic inside GiraphClasses for setting the types is: https://gist.github.com/nitay/2413e54d639e2df80455.
That is either the Computation class is set or you better set the types yourself. There should
really be an else in there that throws but I recall when I tried that there was one or two
cases where we create ICGC before the user/runner code has any chance to run and in that case
they are not set.


> On June 11, 2013, 12:07 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/jython/JythonComputationFactory.java,
line 57
> > <https://reviews.apache.org/r/11709/diff/2/?file=302675#file302675line57>
> >
> >     Wrapped LOG.info please.

Fixed. Btw how come we don't use slf4j? It would clean all this up _and_ be more performant:
http://www.slf4j.org/faq.html#logging_performance


- Nitay


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


On June 10, 2013, 6:55 a.m., Nitay Joffe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11709/
> -----------------------------------------------------------
> 
> (Updated June 10, 2013, 6:55 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> See JIRA
> 
> 
> This addresses bug GIRAPH-683.
>     https://issues.apache.org/jira/browse/GIRAPH-683
> 
> 
> Diffs
> -----
> 
>   giraph-core/pom.xml 3ffe175b675d5be1d878d83d37a04935d30130d6 
>   giraph-core/src/main/java/org/apache/giraph/benchmark/BenchmarkOption.java 23c614b1112dd455a97b6ee7d32188dc5a5fd574

>   giraph-core/src/main/java/org/apache/giraph/benchmark/PageRankBenchmark.java bd2939ec4ce9d3fa373fda667e75df7e0da1cc82

>   giraph-core/src/main/java/org/apache/giraph/conf/AbstractConfOption.java d00f7e908605d8753eb129d0173cf1980034dcc9

>   giraph-core/src/main/java/org/apache/giraph/conf/BooleanConfOption.java c16ec8849641847d53aea8c90dcc0a3fcd646e45

>   giraph-core/src/main/java/org/apache/giraph/conf/ClassConfOption.java 41d120b18bdbbf13c8426ec49f8619d273684620

>   giraph-core/src/main/java/org/apache/giraph/conf/ConfOptionType.java 8f70d904660f14370c2e5c98cc59b6a48ea18135

>   giraph-core/src/main/java/org/apache/giraph/conf/EnumConfOption.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/conf/FloatConfOption.java fa21a281525858208b1ee81be1eafa5ddcb22a0f

>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java 621bb14e4962ec9326c3d58b9de33ad83eb23621

>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 58a3f0179d17cd04f2e69789a6e3838cb22d75eb

>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java 2d0f59cecfdf580609c9d6373c0878c90cede430

>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphTypes.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java
aa5249875de551a341a4baac894c68553cbd6e63 
>   giraph-core/src/main/java/org/apache/giraph/conf/IntConfOption.java de75e9dc4d1696b11251a8e8cac870f6bbf72230

>   giraph-core/src/main/java/org/apache/giraph/conf/LongConfOption.java 0cbc164756de8a422a8f23f01809200b4d569cbf

>   giraph-core/src/main/java/org/apache/giraph/conf/StrConfOption.java 83a583d98820ef946258d2e2a14fc279583b5c5c

>   giraph-core/src/main/java/org/apache/giraph/graph/Computation.java 84158df3ded55360af179e08194e14bc43949c77

>   giraph-core/src/main/java/org/apache/giraph/graph/ComputationFactory.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/graph/ComputationLanguage.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/graph/DefaultComputationFactory.java PRE-CREATION

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

>   giraph-core/src/main/java/org/apache/giraph/job/GiraphConfigurationValidator.java de17157dcbeb123fc43184742a59ed1116070dd6

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

>   giraph-core/src/main/java/org/apache/giraph/jython/package-info.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java a12ef58eec93aa4e801955efcd42f0e5c899275e

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

>   giraph-core/src/main/java/org/apache/giraph/utils/FileUtils.java 442fc9f068c4478706e96a726c5caa2f8fda4563

>   giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java b4920e1a015eda3ea5d5c381d5b0155f38f1891a

>   giraph-core/src/main/java/org/apache/giraph/utils/ReflectionUtils.java 96352bbfc0901e44ffe25bbe2f280a216c797131

>   giraph-core/src/main/resources/org/apache/giraph/benchmark/page-rank.py PRE-CREATION

>   giraph-core/src/test/java/org/apache/giraph/jython/TestJython.java PRE-CREATION 
>   giraph-core/src/test/java/org/apache/giraph/utils/TestReflectionUtils.java PRE-CREATION

>   giraph-core/src/test/resources/org/apache/giraph/jython/count-edges.py PRE-CREATION

>   giraph-hive/src/main/java/org/apache/giraph/hive/HiveGiraphRunner.java 340b4618ce8fdd4ec2303866b2ded632535e40a4

>   pom.xml 0bd85a45f2b26d34c0eacd2ecd7068fefccbb91b 
> 
> Diff: https://reviews.apache.org/r/11709/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Nitay Joffe
> 
>


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