----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12322/#review22872 -----------------------------------------------------------
Not an expert on the Jython stuff, but most of it looks fine. Couple of questions/comments below: giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java <https://reviews.apache.org/r/12322/#comment46613> Isn't this a real error? giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java <https://reviews.apache.org/r/12322/#comment46614> Same question here? giraph-core/src/main/java/org/apache/giraph/conf/JsonStringConfOption.java <https://reviews.apache.org/r/12322/#comment46615> Usually @Override is on the previous line. giraph-core/src/main/java/org/apache/giraph/graph/GraphTaskManager.java <https://reviews.apache.org/r/12322/#comment46636> Not a big fan of Jython specific stuff here. Can it be done in the JythonComputationFactory perchance? giraph-core/src/main/java/org/apache/giraph/jython/DeployedScript.java <https://reviews.apache.org/r/12322/#comment46617> Split line? giraph-core/src/main/java/org/apache/giraph/jython/DeployedScript.java <https://reviews.apache.org/r/12322/#comment46637> Can these be final? giraph-core/src/main/java/org/apache/giraph/jython/DeployedScript.java <https://reviews.apache.org/r/12322/#comment46616> Split line? giraph-core/src/main/java/org/apache/giraph/jython/JythonLoader.java <https://reviews.apache.org/r/12322/#comment46638> 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? - Avery Ching On July 8, 2013, 10:36 p.m., Nitay Joffe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12322/ > ----------------------------------------------------------- > > (Updated July 8, 2013, 10:36 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 > >
