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