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/05/10 23:39:00 UTC
Review Request 21304: Fix KAFKA-1445
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21304/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1445
https://issues.apache.org/jira/browse/KAFKA-1445
Repository: kafka
Description
-------
Mark a partition as ready if some other partitions with the same destination is also ready so it can take a carpool; fix a unchecked or unsafe operations warning
Diffs
-----
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 2d7e52d430fa267ee3689a06f8a621ce5dfd1e33
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java f0152fabbdd44e9f1a24291e84c17edf8379f4fc
clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java f37ab770b1794830154f9908a0156e7e99b4a458
clients/src/test/java/org/apache/kafka/common/utils/AbstractIteratorTest.java 1df226606fad29da47d81d0b8ff36209c3536c06
Diff: https://reviews.apache.org/r/21304/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 21304: Fix KAFKA-1445
Posted by Guozhang Wang <gu...@linkedin.com>.
> On May 12, 2014, 7:35 p.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, line 201
> > <https://reviews.apache.org/r/21304/diff/1/?file=578178#file578178line201>
> >
> > It seems like this only works some of the time. Let's say you have two partitions F and N where F is full and N has a message but isn't full. If you happen to process F then N, N will carpool, but if you process N then F it won't. So you only get 50% of the carpooling you should (on avg). Perhaps I am misunderstanding?
That is correct. Per our off-line discussion I am going to change the return value of ready() to a list of nodes, instead of partitions to resolve this issue.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21304/#review42734
-----------------------------------------------------------
On May 10, 2014, 9:38 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21304/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 9:38 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1445
> https://issues.apache.org/jira/browse/KAFKA-1445
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mark a partition as ready if some other partitions with the same destination is also ready so it can take a carpool; fix a unchecked or unsafe operations warning
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 2d7e52d430fa267ee3689a06f8a621ce5dfd1e33
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java f0152fabbdd44e9f1a24291e84c17edf8379f4fc
> clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java f37ab770b1794830154f9908a0156e7e99b4a458
> clients/src/test/java/org/apache/kafka/common/utils/AbstractIteratorTest.java 1df226606fad29da47d81d0b8ff36209c3536c06
>
> Diff: https://reviews.apache.org/r/21304/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 21304: Fix KAFKA-1445
Posted by Guozhang Wang <gu...@linkedin.com>.
> On May 12, 2014, 7:35 p.m., Jay Kreps wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, line 54
> > <https://reviews.apache.org/r/21304/diff/1/?file=578178#file578178line54>
> >
> > Does this need to be volatile? Do we actually need it? It is a little hard to know how to interpret it...
The reason is that otherwise we need to keep the cluster object inside the accumulator to complete the ready() call in metrics pull operation.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21304/#review42734
-----------------------------------------------------------
On May 10, 2014, 9:38 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21304/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 9:38 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1445
> https://issues.apache.org/jira/browse/KAFKA-1445
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mark a partition as ready if some other partitions with the same destination is also ready so it can take a carpool; fix a unchecked or unsafe operations warning
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 2d7e52d430fa267ee3689a06f8a621ce5dfd1e33
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java f0152fabbdd44e9f1a24291e84c17edf8379f4fc
> clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java f37ab770b1794830154f9908a0156e7e99b4a458
> clients/src/test/java/org/apache/kafka/common/utils/AbstractIteratorTest.java 1df226606fad29da47d81d0b8ff36209c3536c06
>
> Diff: https://reviews.apache.org/r/21304/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 21304: Fix KAFKA-1445
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21304/#review42734
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
<https://reviews.apache.org/r/21304/#comment76592>
Does this need to be volatile? Do we actually need it? It is a little hard to know how to interpret it...
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
<https://reviews.apache.org/r/21304/#comment76593>
It seems like this only works some of the time. Let's say you have two partitions F and N where F is full and N has a message but isn't full. If you happen to process F then N, N will carpool, but if you process N then F it won't. So you only get 50% of the carpooling you should (on avg). Perhaps I am misunderstanding?
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
<https://reviews.apache.org/r/21304/#comment76594>
This linear search may be a bit problematic for large partition counts, no?
Set will be slower in the common case but faster in the extreme case.
Either way we should probably just keep the node id so the comparison is just of a boxed integer rather than a compound object.
- Jay Kreps
On May 10, 2014, 9:38 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21304/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 9:38 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1445
> https://issues.apache.org/jira/browse/KAFKA-1445
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mark a partition as ready if some other partitions with the same destination is also ready so it can take a carpool; fix a unchecked or unsafe operations warning
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 2d7e52d430fa267ee3689a06f8a621ce5dfd1e33
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java f0152fabbdd44e9f1a24291e84c17edf8379f4fc
> clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java f37ab770b1794830154f9908a0156e7e99b4a458
> clients/src/test/java/org/apache/kafka/common/utils/AbstractIteratorTest.java 1df226606fad29da47d81d0b8ff36209c3536c06
>
> Diff: https://reviews.apache.org/r/21304/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>