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 E87E6113B0 for ; Tue, 3 Jun 2014 19:54:13 +0000 (UTC) Received: (qmail 47128 invoked by uid 500); 3 Jun 2014 19:54:13 -0000 Delivered-To: apmail-giraph-dev-archive@giraph.apache.org Received: (qmail 47087 invoked by uid 500); 3 Jun 2014 19:54:13 -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 Received: (qmail 47074 invoked by uid 99); 3 Jun 2014 19:54:13 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 03 Jun 2014 19:54:13 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 169E11D9AB5; Tue, 3 Jun 2014 19:54:05 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============5727655749754980569==" MIME-Version: 1.0 Subject: Re: Review Request 22157: refactor giraph code to support multiple implementations of vertexId data From: "Sergey Edunov" To: "Maja Kabiljo" , "Sergey Edunov" Cc: "giraph" , "Pavan Kumar Athivarapu" Date: Tue, 03 Jun 2014 19:54:05 -0000 Message-ID: <20140603195405.12449.97324@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Sergey Edunov" X-ReviewGroup: giraph X-ReviewRequest-URL: https://reviews.apache.org/r/22157/ X-Sender: "Sergey Edunov" References: <20140602204641.12452.24743@reviews.apache.org> In-Reply-To: <20140602204641.12452.24743@reviews.apache.org> Reply-To: "Sergey Edunov" X-ReviewRequest-Repository: giraph-git --===============5727655749754980569== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > On June 2, 2014, 8:46 p.m., Sergey Edunov wrote: > > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdIterator.java, line 40 > > > > > > How it can be used if it is always throwing exception? It will also cause ByteStructVertexIdMessageBytesIterator and ByteStructVertexIdDataIterator to always throw exception too. Which is counterintuitive. So the question is why do we even need a default constructor in such a case and if we do need, it should be clearly explained that it throws exception here and in all child classes. > > Pavan Kumar Athivarapu wrote: > that's the point - default constructor => no proper initialization. > if it is called - something is wrong. > Also note that a final variable has to be set inside the constructor, so I must implement the default constructor > > what do u think? No, you don't have to implement default constructor because of final variable. You have to initialize it in all available constructors, but that doesn't mean you need default. It's confusing, if class doesn't have default constructor you will get an exception if you try to call it with reflection anyway. > On June 2, 2014, 8:46 p.m., Sergey Edunov wrote: > > giraph-core/src/main/java/org/apache/giraph/utils/ByteUtils.java, line 24 > > > > > > So far it's more like constant class, either way it should be final with private constructor. > > Pavan Kumar Athivarapu wrote: > sure, I can make it private. but being protected is fine - whenever in future something extends this, etc. It doesn't make much sense to extend *Utils class. Anyway going from private to protected is easier than going from protected to private and it is generally advised to give as little access as possible - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22157/#review44543 ----------------------------------------------------------- On June 2, 2014, 7:16 p.m., Pavan Kumar Athivarapu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22157/ > ----------------------------------------------------------- > > (Updated June 2, 2014, 7:16 p.m.) > > > Review request for giraph, Sergey Edunov and Maja Kabiljo. > > > Repository: giraph-git > > > Description > ------- > > currently MessageStores & EdgeStores expect ByteArrayVertexIdData objects. but this is too restrictive, > refactor giraph code to support multiple VertexId structs (for instance ByteBuf, OneMessageToMultipleIds, etc.) > > > Diffs > ----- > > giraph-core/src/main/java/org/apache/giraph/comm/SendEdgeCache.java 8350a55 > giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java 24848db > giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java 54234c5 > giraph-core/src/main/java/org/apache/giraph/comm/SendVertexIdDataCache.java afce3ba > giraph-core/src/main/java/org/apache/giraph/comm/messages/ByteArrayMessagesPerVertexStore.java e8b3b30 > giraph-core/src/main/java/org/apache/giraph/comm/messages/MessageStore.java 2af7642 > giraph-core/src/main/java/org/apache/giraph/comm/messages/MessagesIterable.java 3b22ab3 > giraph-core/src/main/java/org/apache/giraph/comm/messages/OneMessagePerVertexStore.java bb581c0 > giraph-core/src/main/java/org/apache/giraph/comm/messages/out_of_core/DiskBackedMessageStore.java 1a76306 > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/IntByteArrayMessageStore.java cc14c6d > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/IntFloatMessageStore.java 3318610 > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/LongByteArrayMessageStore.java 9e4325f > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/LongDoubleMessageStore.java 76d9ffa > giraph-core/src/main/java/org/apache/giraph/comm/netty/InboundByteCounter.java bcc888d > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java 43c01ce > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/RequestDecoder.java 98a61e6 > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/RequestEncoder.java d379eda > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/RequestServerHandler.java 601cd2f > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/ResponseEncoder.java c0b45fc > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerDataRequest.java 4f80224 > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerEdgesRequest.java 793768a > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerMessagesRequest.java 3ac0962 > giraph-core/src/main/java/org/apache/giraph/comm/requests/WritableRequest.java 181e681 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 2862c3e > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java 6b36418 > giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 95e029d > giraph-core/src/main/java/org/apache/giraph/edge/AbstractEdgeStore.java 80e909d > giraph-core/src/main/java/org/apache/giraph/edge/EdgeStore.java 1150eaf > giraph-core/src/main/java/org/apache/giraph/edge/SimpleEdgeStore.java 6e2a74f > giraph-core/src/main/java/org/apache/giraph/edge/primitives/IntEdgeStore.java c6b5051 > giraph-core/src/main/java/org/apache/giraph/edge/primitives/LongEdgeStore.java d4c44c7 > giraph-core/src/main/java/org/apache/giraph/utils/AbstractVertexIdData.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayIterable.java d14172e > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayIterator.java 28b2dc8 > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdData.java 5c56038 > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdEdges.java 762802b > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdMessages.java 0ac8fdf > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructIterable.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdDataIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdEdgeIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdMessageBytesIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdMessageIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ByteUtils.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/ExtendedByteArrayDataInput.java 0ecea77 > giraph-core/src/main/java/org/apache/giraph/utils/ExtendedByteArrayDataOutput.java 0ff366d > giraph-core/src/main/java/org/apache/giraph/utils/ExtendedDataOutput.java 54ef514 > giraph-core/src/main/java/org/apache/giraph/utils/RepresentativeByteArrayIterable.java 2c24e89 > giraph-core/src/main/java/org/apache/giraph/utils/RepresentativeByteArrayIterator.java d36c94f > giraph-core/src/main/java/org/apache/giraph/utils/RepresentativeByteStructIterable.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/RepresentativeByteStructIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/RequestUtils.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/UnsafeArrayReads.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/UnsafeByteArrayInputStream.java 20ed92b > giraph-core/src/main/java/org/apache/giraph/utils/UnsafeByteArrayOutputStream.java 4b413da > giraph-core/src/main/java/org/apache/giraph/utils/UnsafeReads.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VerboseByteArrayMessageWrite.java 8673732 > giraph-core/src/main/java/org/apache/giraph/utils/VerboseByteStructMessageWrite.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdData.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdDataIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdEdgeIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdEdges.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdIterator.java bad11d6 > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdMessageBytesIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdMessageIterator.java PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/VertexIdMessages.java PRE-CREATION > giraph-core/src/test/java/org/apache/giraph/comm/RequestFailureTest.java 236bc88 > giraph-core/src/test/java/org/apache/giraph/comm/RequestTest.java fcdfa5c > giraph-core/src/test/java/org/apache/giraph/utils/MockUtils.java 97e88f8 > > Diff: https://reviews.apache.org/r/22157/diff/ > > > Testing > ------- > > mvn clean verify > ran a job on the cluster > > > Thanks, > > Pavan Kumar Athivarapu > > --===============5727655749754980569==--