----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31742/#review76985 -----------------------------------------------------------
Thanks for the patch. Could we test out the patch with StressTestLog? core/src/main/scala/kafka/message/ByteBufferMessageSet.scala <https://reviews.apache.org/r/31742/#comment124757> Sometimes, the message size can be large. To avoid memory fragmentation, would it be better to cap the segment size to sth like 256K? So the segment size will be min(messageSetSize, 64KB)? core/src/main/scala/kafka/message/ByteBufferMessageSet.scala <https://reviews.apache.org/r/31742/#comment124756> Instead of passing in a function, I think it will be easier to understand the logic if we just pass in the message list to messageWriter.write(). This way, all writes are inside messageWriter, instead of in two places. core/src/main/scala/kafka/message/ByteBufferMessageSet.scala <https://reviews.apache.org/r/31742/#comment124758> Same comment on the segment size as the above. core/src/main/scala/kafka/message/MessageWriter.scala <https://reviews.apache.org/r/31742/#comment124759> For consistency, size() can just be size. core/src/main/scala/kafka/message/MessageWriter.scala <https://reviews.apache.org/r/31742/#comment124760> Would it be better to name this ReservedOutput? core/src/main/scala/kafka/message/MessageWriter.scala <https://reviews.apache.org/r/31742/#comment124744> Perhaps we can rename off to initOffset and offset to currOffset? core/src/main/scala/kafka/message/MessageWriter.scala <https://reviews.apache.org/r/31742/#comment124743> Shouldn't we test off < b.length and off + len < b.length? core/src/main/scala/kafka/message/MessageWriter.scala <https://reviews.apache.org/r/31742/#comment124745> Perhaps it's clearer to write this as a do .. while? It feels a bit weird that the first statement is adding an amount that is initialized to 0. core/src/test/scala/unit/kafka/message/MessageWriterTest.scala <https://reviews.apache.org/r/31742/#comment124754> Could we just use Random.nextBytes()? core/src/test/scala/unit/kafka/message/MessageWriterTest.scala <https://reviews.apache.org/r/31742/#comment124755> Could we explicitly import the implicits in where it's applied? That way it's clear where it's used. - Jun Rao On March 16, 2015, 10:19 p.m., Yasuhiro Matsuda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31742/ > ----------------------------------------------------------- > > (Updated March 16, 2015, 10:19 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-527 > https://issues.apache.org/jira/browse/KAFKA-527 > > > Repository: kafka > > > Description > ------- > > less byte copies > > > Diffs > ----- > > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala > 9c694719dc9b515fb3c3ae96435a87b334044272 > core/src/main/scala/kafka/message/MessageWriter.scala PRE-CREATION > core/src/test/scala/unit/kafka/message/MessageWriterTest.scala PRE-CREATION > > Diff: https://reviews.apache.org/r/31742/diff/ > > > Testing > ------- > > > Thanks, > > Yasuhiro Matsuda > >
