> 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. > > Yasuhiro Matsuda wrote: > Why the codec should not be NoCompression? The code works with > NoCompression, too.
That's right, it works with NoCompression too. My Bad. > 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? > > Yasuhiro Matsuda wrote: > Is this comment style prohibitted? This class is for internal use with > fairly localized usage. We used to maintain a coding guidence (http://kafka.apache.org/coding-guide.html), but I think we did not do a great job enforcing it and the page itself is also a bit out dated. Jay added the checkstyle package in order to improve on this aspect, but that do not have comments rules in it. I thought the common rules in the code are: 1. Use /* */ for class definitions and user-facing API comments. 2. Use // for in-function comments (no-capitalization). But it is somehow not programmatically enforced. Anyways, let me know if you think that is too picky and we can try be more flexible in terms of commenting. > 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. > > Yasuhiro Matsuda wrote: > This is a contract of OutputStream. Cool. Could we add the optional "override" here? > 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. > > Yasuhiro Matsuda wrote: > 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... Again, we did not do a good job enforcing any sort of such coding style, and it maybe just myself being unreasonable about these rules. I am open to other reviewers taking a look and giving his / her thoughts. 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. > > Yasuhiro Matsuda wrote: > 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. As for now MessageWriter's only public function is write(key, codec) (valueWritefunction), which is used for writing a single message. Also its private functions withCrc32Prefix / withLengthPrefix is only used for message writing. So it is a bit unclear about your motivation in future extensions. Could you elaborate a bit more on that? - Guozhang ----------------------------------------------------------- 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 > >
