> On March 13, 2015, 11:43 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/message/MessageWriter.scala, line 29 > > <https://reviews.apache.org/r/31742/diff/1/?file=884487#file884487line29> > > > > Add a check that codec should not be NoCompression.
Why the codec should not be NoCompression? The code works with NoCompression, too. > On March 13, 2015, 11:43 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/message/MessageWriter.scala, line 97 > > <https://reviews.apache.org/r/31742/diff/1/?file=884487#file884487line97> > > > > Could we use comments in > > > > /** > > * > > */ > > > > format? Is this comment style prohibitted? This class is for internal use with fairly localized usage. > On March 13, 2015, 11:43 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/message/MessageWriter.scala, line 117 > > <https://reviews.apache.org/r/31742/diff/1/?file=884487#file884487line117> > > > > We can just pass in the Byte here. This is a contract of OutputStream. > On March 13, 2015, 11:43 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/message/MessageWriter.scala, line 135 > > <https://reviews.apache.org/r/31742/diff/1/?file=884487#file884487line135> > > > > Better group the private functions together after the public functions. Well, I don't think it is particulary better way to organize code, but if you insist I can change it. Kafka code base doesn't seem to follow that convention... On March 13, 2015, 11:43 p.m., Yasuhiro Matsuda wrote: > > The inheritance of MessageWriter from BufferingOutputStream is a bit > > confusing, since it will always use itself in the writePayload function > > parameter. > > > > I feel it is more clear to read the code if we just let MessageWriter > > contains a var of BufferingOutputStream; and instead of pass in the > > function logic of writing the message, we can just pass in messages and > > offsetCounter in the write() call which will then write the messages itself. It is true that the current code writes only through writePayload. But I wanted MessageWriter to be a subclass of OutputStream to be more generic in case we need to write additional inforation other than messages in future. - Yasuhiro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31742/#review76454 ----------------------------------------------------------- On March 4, 2015, 7:43 p.m., Yasuhiro Matsuda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31742/ > ----------------------------------------------------------- > > (Updated March 4, 2015, 7:43 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 > >
