----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13756/#review25523 -----------------------------------------------------------
giraph-core/src/main/java/org/apache/giraph/io/formats/GiraphTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49963> Giraph style (and Java in general) is to define things where they're used / needed rather than all up top like in C. Please fix. giraph-core/src/main/java/org/apache/giraph/io/formats/GiraphTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49962> inline this with above. giraph-core/src/main/java/org/apache/giraph/io/formats/GiraphTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49964> inline with above giraph-core/src/main/java/org/apache/giraph/io/formats/SrcIdDstIdEdgeValueTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49966> what is the use case for this reversed output? giraph-core/src/main/java/org/apache/giraph/io/formats/SrcIdDstIdEdgeValueTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49965> Use a StringBuilder instead of all this first/second/third stuff. giraph-core/src/main/java/org/apache/giraph/io/formats/TextEdgeOutputFormat.java <https://reviews.apache.org/r/13756/#comment49968> This whole function should be one line: return EDGE_OUTPUT_FORMAT_SUBDIR.get(getConf()); giraph-core/src/main/java/org/apache/giraph/io/formats/TextVertexOutputFormat.java <https://reviews.apache.org/r/13756/#comment49969> return VERTEX_OUTPUT_FORMAT_SUBDIR.get(getConf()); giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeOutputFormat.java <https://reviews.apache.org/r/13756/#comment49971> final giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeOutputFormat.java <https://reviews.apache.org/r/13756/#comment49970> remove space giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeOutputFormat.java <https://reviews.apache.org/r/13756/#comment49972> remove extra spaces in all these methods giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java <https://reviews.apache.org/r/13756/#comment49973> remove final modifiers here, we don't use them in method variables giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java <https://reviews.apache.org/r/13756/#comment49974> Should we track and add something in here about number of edges saved instead of just vertices, seeing as this is all about edge writing? giraph-core/src/test/java/org/apache/giraph/io/TestSrcIdDstIdEdgeValueTextOutputFormat.java <https://reviews.apache.org/r/13756/#comment49975> srcIdDstIdEdgeValueTestWorker, only classes begin with upper case Overall logic looks good, but there is lots of style issues. Please fix them. Did you run checkstyle and everything by the way? - Nitay Joffe On Aug. 23, 2013, 1:34 a.m., Armando wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13756/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2013, 1:34 a.m.) > > > Review request for giraph, Armando, Claudio Martella, and Nitay Joffe. > > > Bugs: GIRAPH-732 > https://issues.apache.org/jira/browse/GIRAPH-732 > > > Repository: giraph-git > > > Description > ------- > > This patch is a possible solution to provide an EdgeOutputFormat API in > Giraph. > The idea is to add the same logic as the one for the vertices for edges. For > this reason I add saveEdges after saveVertices. > This requires the user to have to different paths for the resulting files. > For this reason I have implemented TextEdgeOutputFormat > and modified TextVertexOutputFormat so that a subdirectory can be specified. > The code I provide should be compliant with previous versions of hadoop as > well as YARN (I was yet not able to test it on YARN). > It is also retro-compatible since when not specified, TextVertexOutputFormat > behaves as before. > I am also providing an actual usable implementation with the associated tests > (SrcIdDstIdEdgeValueOutputFormat). > Also very interesting is the fact that this implementation is totally > transparent to the classes implementing TextVertexOutputFormat. > > passed "maven verify" > > > Diffs > ----- > > giraph-core/src/main/java/org/apache/giraph/GiraphRunner.java 1bd79b5 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java 71fe885 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java > 23bcd32 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java > c276c2a > > giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java > 49a2ebc > giraph-core/src/main/java/org/apache/giraph/io/EdgeOutputFormat.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/io/EdgeWriter.java PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/GiraphTextOutputFormat.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/SrcIdDstIdEdgeValueTextOutputFormat.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/TextEdgeOutputFormat.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/TextVertexOutputFormat.java > c91d543 > > giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeOutputFormat.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java > 745764b > giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java > da1e7fb > > giraph-core/src/test/java/org/apache/giraph/io/TestSrcIdDstIdEdgeValueTextOutputFormat.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/13756/diff/ > > > Testing > ------- > > > Thanks, > > Armando > >
