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 <gu...@linkedin.com> on 2014/04/24 00:20:49 UTC
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round two
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/
-----------------------------------------------------------
(Updated April 23, 2014, 10:20 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1409: Address Jun's comments round two
Bugs: KAFKA-1409
https://issues.apache.org/jira/browse/KAFKA-1409
Repository: kafka
Description (updated)
-------
Fix KAFKA-1409: Address Jun's comments round two
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
Diff: https://reviews.apache.org/r/20616/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round two
Posted by Guozhang Wang <gu...@linkedin.com>.
> On April 24, 2014, 12:44 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/log/Log.scala, lines 275-276
> > <https://reviews.apache.org/r/20616/diff/4/?file=566280#file566280line275>
> >
> > We probably should use validMessages here.
I actually thought about this. The thing is that the bytes that gets trimmed is not recorded anywhere. So when we reject these messages in the re-check after de-recompression, it would be better to record the whole message bytes just to account those trimmed bytes.
> On April 24, 2014, 12:44 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/log/Log.scala, lines 346-347
> > <https://reviews.apache.org/r/20616/diff/4/?file=566280#file566280line346>
> >
> > This requires computing the checksum and can be expensive. We probably should move it to after the message size check.
If the message-too-large happens, then at most one checksum computation can be wasted by having this before the size validation (once the exception is thrown, rest of messages will be dropped immediately), but I will move it after the size validation just in case.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41250
-----------------------------------------------------------
On April 23, 2014, 10:20 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 10:20 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round three
Posted by Jun Rao <ju...@gmail.com>.
> On April 24, 2014, 12:44 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/log/Log.scala, lines 275-276
> > <https://reviews.apache.org/r/20616/diff/4/?file=566280#file566280line275>
> >
> > We probably should use validMessages here.
>
> Guozhang Wang wrote:
> I actually thought about this. The thing is that the bytes that gets trimmed is not recorded anywhere. So when we reject these messages in the re-check after de-recompression, it would be better to record the whole message bytes just to account those trimmed bytes.
That's fine. Perhaps we can add a comment to explain this?
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41250
-----------------------------------------------------------
On April 24, 2014, 11:26 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 11:26 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Update comments
>
>
> Incorporate Jun's comments round three
>
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round two
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41250
-----------------------------------------------------------
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74690>
We probably should use validMessages here.
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74688>
The comment needs to be updated.
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74687>
Could this be changed to validBytes too?
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74689>
This requires computing the checksum and can be expensive. We probably should move it to after the message size check.
- Jun Rao
On April 23, 2014, 10:20 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 23, 2014, 10:20 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun and Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
> On April 25, 2014, 5:32 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/log/Log.scala, lines 274-275
> > <https://reviews.apache.org/r/20616/diff/6/?file=568559#file568559line274>
> >
> > entrySNnnnNize?
Never search for classes while doing "git commit" .... sorry about that.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41490
-----------------------------------------------------------
On April 25, 2014, 5:22 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 25, 2014, 5:22 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Neha's comments
>
>
> Update comments
>
>
> Incorporate Jun's comments round three
>
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun and Neha's comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41490
-----------------------------------------------------------
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74934>
entrySnnnnNize?
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74936>
entrySNnnnNize?
- Jun Rao
On April 25, 2014, 5:22 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 25, 2014, 5:22 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Neha's comments
>
>
> Update comments
>
>
> Incorporate Jun's comments round three
>
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun and Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/
-----------------------------------------------------------
(Updated April 25, 2014, 5:44 p.m.)
Review request for kafka.
Bugs: KAFKA-1409
https://issues.apache.org/jira/browse/KAFKA-1409
Repository: kafka
Description (updated)
-------
typo fix
Address Neha's comments
Update comments
Incorporate Jun's comments round three
Fix KAFKA-1409: Address Jun's comments round two
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
Diff: https://reviews.apache.org/r/20616/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20616: Fix KAFKA-1409: Address Jun and Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/
-----------------------------------------------------------
(Updated April 25, 2014, 5:22 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1409: Address Jun and Neha's comments
Bugs: KAFKA-1409
https://issues.apache.org/jira/browse/KAFKA-1409
Repository: kafka
Description (updated)
-------
Address Neha's comments
Update comments
Incorporate Jun's comments round three
Fix KAFKA-1409: Address Jun's comments round two
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
Diff: https://reviews.apache.org/r/20616/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round three
Posted by Guozhang Wang <gu...@linkedin.com>.
> On April 25, 2014, 12:24 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/log/Log.scala, line 275
> > <https://reviews.apache.org/r/20616/diff/5/?file=568178#file568178line275>
> >
> > Can we compute messages.sizeInBytes just once instead of twice?
messages.sizeInBytes just calls buffer.limit, which does not do any computation.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41421
-----------------------------------------------------------
On April 24, 2014, 11:26 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 11:26 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Update comments
>
>
> Incorporate Jun's comments round three
>
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round three
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/#review41421
-----------------------------------------------------------
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74859>
Can we compute messages.sizeInBytes just once instead of twice?
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74861>
same here
core/src/main/scala/kafka/log/Log.scala
<https://reviews.apache.org/r/20616/#comment74862>
You can use messageSize instead of recomputing the message entry size again
- Neha Narkhede
On April 24, 2014, 11:26 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20616/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 11:26 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1409
> https://issues.apache.org/jira/browse/KAFKA-1409
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Update comments
>
>
> Incorporate Jun's comments round three
>
>
> Fix KAFKA-1409: Address Jun's comments round two
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
> core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
> core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
> core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
> core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
>
> Diff: https://reviews.apache.org/r/20616/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20616: Fix KAFKA-1409: Address Jun's comments round three
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20616/
-----------------------------------------------------------
(Updated April 24, 2014, 11:26 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1409: Address Jun's comments round three
Bugs: KAFKA-1409
https://issues.apache.org/jira/browse/KAFKA-1409
Repository: kafka
Description (updated)
-------
Update comments
Incorporate Jun's comments round three
Fix KAFKA-1409: Address Jun's comments round two
Diffs (updated)
-----
core/src/main/scala/kafka/cluster/Partition.scala c08eab0bc35aa2b0caa0dfc1987b4face4315d3b
core/src/main/scala/kafka/log/Log.scala 46df8d99d977a3b010a9b9f4698187fa9bfb2498
core/src/main/scala/kafka/message/MessageSet.scala a1b5c63b4d9deaa2556427d995c41841838cf581
core/src/main/scala/kafka/server/KafkaApis.scala bb0359d933356b8fcc2c8df22ce6ea9cb838d6a2
core/src/main/scala/kafka/server/KafkaRequestHandler.scala f11f6e2c82b016f4dd3e92466ae1d2ede19feb30
Diff: https://reviews.apache.org/r/20616/diff/
Testing
-------
Thanks,
Guozhang Wang