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