Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review43172 --- Ship it! Ship It! - Jay Kreps On May 15, 2014, 10:19 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 15, 2014, 10:19 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review43157 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment77213 Instead of a ListNode use a HashSetInteger to avoid the O(N) comparisons of a multifield object. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment77203 Is Node.UNKNOWN better than null? Also, please avoid single line statements like if(x) y; clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment77205 Linear search doing a full .equals comparison on each Node object. clients/src/main/java/org/apache/kafka/common/Node.java https://reviews.apache.org/r/21398/#comment77202 This should either be unknown or UNKNOWN. - Jay Kreps On May 14, 2014, 11:28 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 14, 2014, 11:28 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
On May 15, 2014, 8:47 p.m., Jay Kreps wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, line 201 https://reviews.apache.org/r/21398/diff/4/?file=582079#file582079line201 Instead of a ListNode use a HashSetInteger to avoid the O(N) comparisons of a multifield object. Actually thinking about it this may not be a big deal as the comparison should short circuit on the == comparison. So for small sets of nodes this may be better. - Jay --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review43157 --- On May 14, 2014, 11:28 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 14, 2014, 11:28 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 15, 2014, 10:15 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
On May 15, 2014, 8:47 p.m., Jay Kreps wrote:
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java,
line 210
https://reviews.apache.org/r/21398/diff/4/?file=582079#file582079line210
Is Node.UNKNOWN better than null?
Also, please avoid single line statements like
if(x) y;
The original reason that I thought to not use null is because due to Node.equal
function, null != null, and List.contains use equal function. But then when I
check ArrayList.contains implementation it is actually fine (it used indexOf
function):
-
public int indexOf(Object o) {
if (o == null) {
for (int i = 0; i size; i++)
if (elementData[i]==null)
return i;
} else {
for (int i = 0; i size; i++)
if (o.equals(elementData[i]))
return i;
}
return -1;
}
On May 15, 2014, 8:47 p.m., Jay Kreps wrote:
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java,
line 211
https://reviews.apache.org/r/21398/diff/4/?file=582079#file582079line211
Linear search doing a full .equals comparison on each Node object.
Get it, I think we can use a hashset in this case then.
- Guozhang
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21398/#review43157
---
On May 14, 2014, 11:28 p.m., Guozhang Wang wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21398/
---
(Updated May 14, 2014, 11:28 p.m.)
Review request for kafka.
Bugs: KAFKA-1445
https://issues.apache.org/jira/browse/KAFKA-1445
Repository: kafka
Description
---
0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list
of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon
node placeholder, and processReadNodes would force metadata update in this
case;\n 3. Drain would take a list of nodes and drain the batches per node
until the max request size is reached;\n 4. Collocate would not be just
tranform batches per node into a producer request;\n 5. Corresponding unit
test changes; \n 6. One minor compilation warning fix
Diffs
-
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
a6423f4b37a57f0290e2048b764de1218470f4f7
clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java
6a0f3b27f754d340aa133218204470a822d4d747
clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java
f114ffd84c502b6c4070f77d2f6a47ef478b30aa
clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java
fbb732a57522109ac0e0eaf5c87b50cbd3a7f767
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/main/java/org/apache/kafka/common/Cluster.java
426bd1eec708979149cbd6fa3959e6f9e73c7e0e
clients/src/main/java/org/apache/kafka/common/Node.java
0e47ff3ff0e055823ec5a5aa4839d25b0fac8374
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/21398/diff/
Testing
---
unit tests
Thanks,
Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review43122 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment77132 There could be ready partitions after their nodes are identified as ready. So we need to account for those. Probably the check for readyNodes can be moved to before line 215. - Jun Rao On May 14, 2014, 11:28 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 14, 2014, 11:28 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
On May 14, 2014, 4:31 a.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, lines 204-247 https://reviews.apache.org/r/21398/diff/2/?file=580819#file580819line204 Could the two loops be merged into a single loop? The first loop actually loops over the nodes, and for each node check their TopicAndPartition's corresponding PartitionInfo and will break for the first ready partition. The second loop over the PartitionInfo directly and will also break for the first ready partition with unknown leader. It will be a bit complex to merge these two. - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review42921 --- On May 13, 2014, 6:25 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:25 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 14, 2014, 11:24 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description (updated) --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 14, 2014, 11:28 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready nodes may contain a Unknon node placeholder, and processReadNodes would force metadata update in this case;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java a6423f4b37a57f0290e2048b764de1218470f4f7 clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 6a0f3b27f754d340aa133218204470a822d4d747 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java f114ffd84c502b6c4070f77d2f6a47ef478b30aa clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
On May 13, 2014, 6:37 p.m., Timothy Chen wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java, line 323 https://reviews.apache.org/r/21398/diff/2/?file=580820#file580820line323 Should we only call forceUpdate once if we get multiple unknown nodes? The benefit of forcing metadata update whenever there is one ready partition with unknown leader is to minimize latency for these cases. Since partitions with unknown leaders (e.g. newly created topic, extended partitions, etc) would be a rare case, I think this would not add too much load on metadata request and a good tradeoff for latency. - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review42869 --- On May 13, 2014, 6:25 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:25 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review42982 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java https://reviews.apache.org/r/21398/#comment76966 Got it! - Timothy Chen On May 13, 2014, 6:25 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:25 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review42985 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment76971 I want to second Jun's comment. I think this method is far too complex. We need to find a way to simplify this. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java https://reviews.apache.org/r/21398/#comment76972 Metadata updates cannot cannot cannot bleed into this class. This class isn't about updating metadata. clients/src/main/java/org/apache/kafka/common/Cluster.java https://reviews.apache.org/r/21398/#comment76975 There are two methods with the same name that do two different things. Can we make them something like partitionsForTopic(topic) and partitionsForNode(node) ? - Jay Kreps On May 13, 2014, 6:25 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:25 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:19 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description (updated) --- 0. Add the partitionsForNode index in Cluster; 1. Ready would return a list of ready nodes instead of partitions; 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh; 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached; 4. Collocate would not be just transform batches per node into a producer request; 5. Corresponding unit test changes; 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing (updated) --- unit tests Thanks, Guozhang Wang
Re: Review Request 21398: Fix KAFKA-1445 v2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/#review42869 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java https://reviews.apache.org/r/21398/#comment76775 Should we only call forceUpdate once if we get multiple unknown nodes? - Timothy Chen On May 13, 2014, 6:25 p.m., Guozhang Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21398/ --- (Updated May 13, 2014, 6:25 p.m.) Review request for kafka. Bugs: KAFKA-1445 https://issues.apache.org/jira/browse/KAFKA-1445 Repository: kafka Description --- 0. Add the partitionsForNode index in Cluster;\n 1. Ready would return a list of ready nodes instead of partitions;\n 2. Ready would also check if there is any ready partitions with unknown leader, if yes indicate the processReadyNode to force metadata refresh;\n 3. Drain would take a list of nodes and drain the batches per node until the max request size is reached;\n 4. Collocate would not be just tranform batches per node into a producer request;\n 5. Corresponding unit test changes; \n 6. One minor compilation warning fix Diffs - clients/src/main/java/org/apache/kafka/clients/producer/internals/Partitioner.java fbb732a57522109ac0e0eaf5c87b50cbd3a7f767 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/main/java/org/apache/kafka/common/Cluster.java 426bd1eec708979149cbd6fa3959e6f9e73c7e0e clients/src/main/java/org/apache/kafka/common/Node.java 0e47ff3ff0e055823ec5a5aa4839d25b0fac8374 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/21398/diff/ Testing --- unit tests Thanks, Guozhang Wang
