You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <wa...@gmail.com> on 2015/03/07 00:48:43 UTC
Review Request 31816: Fix for KAFKA-527
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-527
https://issues.apache.org/jira/browse/KAFKA-527
Repository: kafka
Description
-------
Avoid double copying on decompress
Diffs
-----
core/src/main/scala/kafka/consumer/ConsumerIterator.scala ac491b4da2583ef7227c67f5b8bc0fd731d705c3
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 788c7864bc881b935975ab4a4e877b690e65f1f1
core/src/test/scala/unit/kafka/message/MessageCompressionTest.scala 6f0addcea64f1e78a4de50ec8135f4d02cebd305
core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea06753e5358aa341c589ca7a7704317e29c
Diff: https://reviews.apache.org/r/31816/diff/
Testing
-------
Unit tests
Thanks,
Guozhang Wang
Re: Review Request 31816: Fix for KAFKA-527
Posted by Guozhang Wang <wa...@gmail.com>.
> On March 21, 2015, 1:42 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, lines 241-243
> > <https://reviews.apache.org/r/31816/diff/1/?file=888022#file888022line241>
> >
> > In the normal path, this should only happen when reading the offset. So we should probably handle the EOFException there. If we get EOFException in other places, it should be treated as an error.
I think EOF can be thrown in both reading the header (offset, size) or the message itself, like in the old internalIterator.makeNextOuter, we check if remaining bytes are not sufficient and return allDone for both reading the header and the message content.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/#review77328
-----------------------------------------------------------
On March 6, 2015, 11:48 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31816/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 11:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-527
> https://issues.apache.org/jira/browse/KAFKA-527
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Avoid double copying on decompress
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerIterator.scala ac491b4da2583ef7227c67f5b8bc0fd731d705c3
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 788c7864bc881b935975ab4a4e877b690e65f1f1
> core/src/test/scala/unit/kafka/message/MessageCompressionTest.scala 6f0addcea64f1e78a4de50ec8135f4d02cebd305
> core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea06753e5358aa341c589ca7a7704317e29c
>
> Diff: https://reviews.apache.org/r/31816/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 31816: Fix for KAFKA-527
Posted by Guozhang Wang <wa...@gmail.com>.
> On March 21, 2015, 1:42 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/message/ByteBufferMessageSet.scala, line 208
> > <https://reviews.apache.org/r/31816/diff/1/?file=888022#file888022line208>
> >
> > Can we just use wrapperMessage directly?
I think we can, as we are just accessing its underlying buffer for reading data; it is the same pattern as we previously do in ByteBufferMessageSet.decompress().
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/#review77328
-----------------------------------------------------------
On March 6, 2015, 11:48 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31816/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 11:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-527
> https://issues.apache.org/jira/browse/KAFKA-527
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Avoid double copying on decompress
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerIterator.scala ac491b4da2583ef7227c67f5b8bc0fd731d705c3
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 788c7864bc881b935975ab4a4e877b690e65f1f1
> core/src/test/scala/unit/kafka/message/MessageCompressionTest.scala 6f0addcea64f1e78a4de50ec8135f4d02cebd305
> core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea06753e5358aa341c589ca7a7704317e29c
>
> Diff: https://reviews.apache.org/r/31816/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 31816: Fix for KAFKA-527
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/#review77328
-----------------------------------------------------------
Thanks for the patch. A few comments below.
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31816/#comment125239>
Can we just use wrapperMessage directly?
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31816/#comment125242>
This is inconsistent as in line 160 and 161. We should do the same check on size and throw the same type of exception.
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31816/#comment125241>
In the normal path, this should only happen when reading the offset. So we should probably handle the EOFException there. If we get EOFException in other places, it should be treated as an error.
- Jun Rao
On March 6, 2015, 11:48 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31816/
> -----------------------------------------------------------
>
> (Updated March 6, 2015, 11:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-527
> https://issues.apache.org/jira/browse/KAFKA-527
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Avoid double copying on decompress
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerIterator.scala ac491b4da2583ef7227c67f5b8bc0fd731d705c3
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 788c7864bc881b935975ab4a4e877b690e65f1f1
> core/src/test/scala/unit/kafka/message/MessageCompressionTest.scala 6f0addcea64f1e78a4de50ec8135f4d02cebd305
> core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea06753e5358aa341c589ca7a7704317e29c
>
> Diff: https://reviews.apache.org/r/31816/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 31816: Fix decompression regarding KAFKA-572
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/#review77955
-----------------------------------------------------------
Ship it!
Thanks for the patch. Just a couple of minor comments below. +1. Also, it seems that you need to rebase the patch.
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31816/#comment126319>
decompress => decompresses
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala
<https://reviews.apache.org/r/31816/#comment126317>
It may be useful to indicate in the message string that this is from the deep iterator. This way, we can distinguish it from the message in line 180.
- Jun Rao
On March 25, 2015, 8:26 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31816/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 8:26 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-527
> https://issues.apache.org/jira/browse/KAFKA-527
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Incorporated Jun and Joel's comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerIterator.scala 78fbf75651583e390258af2d9f09df6911a97b59
> core/src/main/scala/kafka/log/LogSegment.scala ac9643423a28d189133705ba69b16cfce23f0049
> core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 9c694719dc9b515fb3c3ae96435a87b334044272
> core/src/main/scala/kafka/tools/DumpLogSegments.scala fe2cc11b75f370beb9cb87ebc9ed01b63fd65f87
> core/src/test/scala/unit/kafka/log/LogTest.scala 8cd5f2fa4a1a536c3983c5b6eac3d80de49d5a94
> core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala b5208a5f1186bc089cd89527c1eb7f95b2e76c75
>
> Diff: https://reviews.apache.org/r/31816/diff/
>
>
> Testing
> -------
>
> Unit tests
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 31816: Fix decompression regarding KAFKA-572
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31816/
-----------------------------------------------------------
(Updated March 25, 2015, 8:26 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix decompression regarding KAFKA-572
Bugs: KAFKA-527
https://issues.apache.org/jira/browse/KAFKA-527
Repository: kafka
Description (updated)
-------
Incorporated Jun and Joel's comments
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ConsumerIterator.scala 78fbf75651583e390258af2d9f09df6911a97b59
core/src/main/scala/kafka/log/LogSegment.scala ac9643423a28d189133705ba69b16cfce23f0049
core/src/main/scala/kafka/message/ByteBufferMessageSet.scala 9c694719dc9b515fb3c3ae96435a87b334044272
core/src/main/scala/kafka/tools/DumpLogSegments.scala fe2cc11b75f370beb9cb87ebc9ed01b63fd65f87
core/src/test/scala/unit/kafka/log/LogTest.scala 8cd5f2fa4a1a536c3983c5b6eac3d80de49d5a94
core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala b5208a5f1186bc089cd89527c1eb7f95b2e76c75
Diff: https://reviews.apache.org/r/31816/diff/
Testing
-------
Unit tests
Thanks,
Guozhang Wang