Return-Path: X-Original-To: apmail-giraph-dev-archive@www.apache.org Delivered-To: apmail-giraph-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 6580AF18A for ; Wed, 20 Mar 2013 22:59:07 +0000 (UTC) Received: (qmail 24611 invoked by uid 500); 20 Mar 2013 22:59:07 -0000 Delivered-To: apmail-giraph-dev-archive@giraph.apache.org Received: (qmail 24528 invoked by uid 500); 20 Mar 2013 22:59:07 -0000 Mailing-List: contact dev-help@giraph.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@giraph.apache.org Delivered-To: mailing list dev@giraph.apache.org Delivered-To: moderator for dev@giraph.apache.org Received: (qmail 10649 invoked by uid 99); 20 Mar 2013 22:56:02 -0000 Content-Type: multipart/alternative; boundary="===============1514357591735757678==" MIME-Version: 1.0 Subject: Re: Review Request: In memory testing framework added From: "Alessandro Presta" To: "giraph" , "Veselin Stoyanov" , "Alessandro Presta" Date: Wed, 20 Mar 2013 22:56:01 -0000 Message-ID: <20130320225601.6686.54283@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Alessandro Presta" X-ReviewGroup: giraph X-ReviewRequest-URL: https://reviews.apache.org/r/10052/ X-Sender: "Alessandro Presta" References: <20130320220531.6650.57558@reviews.apache.org> In-Reply-To: <20130320220531.6650.57558@reviews.apache.org> Reply-To: "Alessandro Presta" --===============1514357591735757678== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10052/#review18177 ----------------------------------------------------------- Thanks for taking the time to work on this! Please see the inline comments. My main concern is that I don't see how we get edge values. It seems to me = that there should be no need for InMemoryEdgeInput, and instead we should s= tore edge values in TestGraph (probably adding an addEdge() method too). Also, I think this functionality could be achieved more simply by storing t= he Vertex objects in TestGraph (probably in a Map), and having I= nMemoryVertexInput simply return references to those vertices. There will b= e no need for InMemoryVertexOutput, as the vertices will be modified in-pla= ce and InternalVertexRunner#run() will just return the same TestGraph. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java Usually we put this all in one line. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java Why is this class needed anyway? Isn't a VertexInputFormat sufficient o= nce we have built our TestGraph? You just need to handle edge values in the= re too. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java Why do you need your own DummyInputSplit? Can't you use BspInputSplit l= ike GeneratedVertexInputFormat does? giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java Why is this in terms of WritableComparable and Writable instead of I an= d E? giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.java What's going on with this iterator? You get it from an empty list. Late= r you have a commented assignment. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java See above. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java See above. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java I would call this "getEdges", maybe "createEdges". Also, looks like it = could be converted to a static method. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java Why are we assuming there are no edge values? giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java Leftover. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.java Assigning the format's conf from the reader doesn't feel right. You can instead make the format implement ImmutableClassesGiraphConfigu= rable, so that Giraph will provide it with the conf upon instantiation. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java See above. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java I think this would be easier to use if it returned a Map ins= tead. That way we can easily look up just the vertices we're interested in,= as well as iterate if we want to check a condition against all vertices. = Even better would be to have TestGraph itself implement a sort of map-l= ike interface providing lookup and iteration over values, and skip the outp= ut format entirely. giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.java There's a class called ImmutableOutputCommitter (not sure why it's call= ed like that) that's the same as this anonymous class. giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.java This comment is not completely relevant, since there is no input file o= r output folder, right? giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java This is really a container of vertex values, not vertices. giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java I think we should be able to skip this intermediate representation, and= directly store the vertices in a Map. Then, InMemoryVertexInput can simply return references to the already-c= reated vertices in TestGraph. No output format will be needed then, as the vertices will be modified = in place. That way InternalVertexRunner#run() can simply return the same TestGrap= h reference, and a user can do any checks directly against that. giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java This is really a container of neighbor ids, not edges. giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java Can you switch to more descriptive parameter names, as in the rest of t= he codebase? In this case, since it's clear that we're talking about a vertex, I'd b= e happy with "id" and "value" instead of "i" and "v". giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java It would be useful to also have an addEdge(I sourceVertexId, targetVert= exId) method. If the vertex exists, it should add the edge. If the vertex doesn't exi= st, it should create it with a default value and then add the edge. giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java This is a bit misleading because it doesn't return an iterator over the= edges, but over the neighbor ids. Also, unless you need it to be an ArrayList in the caller, you can retu= rn a more generic Iterable. giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponent= sVertexTestInMemory.java Trailing space. giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponent= sVertexTestInMemory.java Can't you rewrite this more compactly as a chain of calls? = graph.addVertex(...) .addVertex(...) .addVertex(...) ... giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponent= sVertexTestInMemory.java Trailing spaces. - Alessandro Presta On March 20, 2013, 10:05 p.m., Veselin Stoyanov wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10052/ > ----------------------------------------------------------- > = > (Updated March 20, 2013, 10:05 p.m.) > = > = > Review request for giraph. > = > = > Description > ------- > = > A framework for running small test graphs in memory. > = > = > Diffs > ----- > = > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryEdgeInput.jav= a PRE-CREATION = > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexInput.j= ava PRE-CREATION = > giraph-core/src/main/java/org/apache/giraph/utils/InMemoryVertexOutput.= java PRE-CREATION = > giraph-core/src/main/java/org/apache/giraph/utils/InternalVertexRunner.= java 7e0b955998a5f1890912819fa2ca74fbbf46fedd = > giraph-core/src/main/java/org/apache/giraph/utils/TestGraph.java PRE-CR= EATION = > giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedCompo= nentsVertexTestInMemory.java PRE-CREATION = > = > Diff: https://reviews.apache.org/r/10052/diff/ > = > = > Testing > ------- > = > = > Thanks, > = > Veselin Stoyanov > = > --===============1514357591735757678==--