> On June 2, 2014, 8:46 p.m., Sergey Edunov wrote: > > giraph-core/src/main/java/org/apache/giraph/utils/ByteStructVertexIdIterator.java, > > line 40 > > <https://reviews.apache.org/r/22157/diff/1/?file=601920#file601920line40> > > > > 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 > > <https://reviews.apache.org/r/22157/diff/1/?file=601923#file601923line24> > > > > 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 > >
