You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Timothy Chen <tn...@apache.org> on 2014/04/02 22:44:10 UTC
Review Request 19957: Patch for KAFKA-1356
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39605
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On April 4, 2014, 9:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:40 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 5, 2014, 12:11 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/api/TopicMetadata.scala, line 35
> > <https://reviews.apache.org/r/19957/diff/3/?file=549380#file549380line35>
> >
> > could you confirm why we need to fill the array vs create an empty one of size numPartitions?
I realized I was using scala's immutable array when I was trying it, with a mutable array it works. Fixing it now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39618
-----------------------------------------------------------
On April 4, 2014, 9:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:40 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39618
-----------------------------------------------------------
core/src/main/scala/kafka/api/TopicMetadata.scala
<https://reviews.apache.org/r/19957/#comment72071>
could you confirm why we need to fill the array vs create an empty one of size numPartitions?
- Neha Narkhede
On April 4, 2014, 9:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 9:40 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39652
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On April 6, 2014, 8:46 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 6, 2014, 8:46 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39840
-----------------------------------------------------------
Thanks for the patch. Some additional comments.
1. The introduction of MetadataCache is a good idea and provides better code isolation.
1.1 Could we move aliveBroker in it too?
1.2 Could we make it a private class?
1.3 Could we more the readwriteLock and methods that access the lock inside this class too? This way, the usage of the lock is only limited inside the class.
2. I am concerned of the overhead of the lock in ensureTopicExists(). This method is called in every produce and fetch request and is a global lock. I think the purpose is to disable access to a topic when a topic is deleted. However, even if we don't call ensureTopicExists(), once a replica is deleted by the ReplicaManager, all requests will get the UnknownTopicOrPartitonException. ReplicaManager may find out that a partition is being deleted a bit later. But this is not a big issue since we just accumulated a bit more data which will be deleted soon.
- Jun Rao
On April 8, 2014, 8:38 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 8, 2014, 8:38 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala c8c02ced543a6278ba5b1edfa73a370f1edb1891
> core/src/main/scala/kafka/server/KafkaApis.scala 705d87ed874f2363e6d9d6ea1143bc6035a0a9d6
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
(Updated April 8, 2014, 8:38 a.m.)
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs (updated)
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/controller/KafkaController.scala c8c02ced543a6278ba5b1edfa73a370f1edb1891
core/src/main/scala/kafka/server/KafkaApis.scala 705d87ed874f2363e6d9d6ea1143bc6035a0a9d6
core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
(Updated April 6, 2014, 8:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs (updated)
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39635
-----------------------------------------------------------
Just one minor comment. Other than that, it looks great! We can check it in after this comment is addressed
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/19957/#comment72118>
It's worth adding a get API to the MetadataCache and make cache private.
- Neha Narkhede
On April 5, 2014, 12:45 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 5, 2014, 12:45 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
> core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
> core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
(Updated April 5, 2014, 12:45 a.m.)
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs (updated)
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
(Updated April 4, 2014, 9:40 p.m.)
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs (updated)
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
core/src/test/scala/unit/kafka/admin/AdminTest.scala d5644ea40ec7678b975c4775546b79fcfa9f64b7
core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca
core/src/test/scala/unit/kafka/utils/TestUtils.scala 2054c25bbced6bd90c092a1974975732ad346146
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Guozhang Wang <gu...@linkedin.com>.
> On April 3, 2014, 5:12 a.m., Guozhang Wang wrote:
> > Just curious: which part of the code is the main contribution to the handling overhead?
>
> Joel Koshy wrote:
> Tim can correct me if I'm wrong but I think the overhead was largely in the sort.
>
> Thanks again for the patch. I'll get this checked in.
>
> Timothy Chen wrote:
> From profiling with 1000 topics actually the largest contribution looks like was the metadataCache.map(_.topic) call, which originally we call for each topic that is being requested. I moved it so that we just do it once.
>
> Timothy Chen wrote:
> Sorry left out an important detail, the most time was spent in HashSet.updated0 call which looks like it's from metadataCache.map(_.topic).toSet. The construction of the new set seems to be expensive and we do it for each entry of topic and partition.
Thanks Tim. Did the profiling result look the same for less than 5 topics? I remembered last time we saw the overhead ratio is much larger when we have just a few topics.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39402
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 3, 2014, 5:12 a.m., Guozhang Wang wrote:
> > Just curious: which part of the code is the main contribution to the handling overhead?
>
> Joel Koshy wrote:
> Tim can correct me if I'm wrong but I think the overhead was largely in the sort.
>
> Thanks again for the patch. I'll get this checked in.
>From profiling with 1000 topics actually the largest contribution looks like was the metadataCache.map(_.topic) call, which originally we call for each topic that is being requested. I moved it so that we just do it once.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39402
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 3, 2014, 5:12 a.m., Guozhang Wang wrote:
> > Just curious: which part of the code is the main contribution to the handling overhead?
>
> Joel Koshy wrote:
> Tim can correct me if I'm wrong but I think the overhead was largely in the sort.
>
> Thanks again for the patch. I'll get this checked in.
>
> Timothy Chen wrote:
> From profiling with 1000 topics actually the largest contribution looks like was the metadataCache.map(_.topic) call, which originally we call for each topic that is being requested. I moved it so that we just do it once.
Sorry left out an important detail, the most time was spent in HashSet.updated0 call which looks like it's from metadataCache.map(_.topic).toSet. The construction of the new set seems to be expensive and we do it for each entry of topic and partition.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39402
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Joel Koshy <jj...@gmail.com>.
> On April 3, 2014, 5:12 a.m., Guozhang Wang wrote:
> > Just curious: which part of the code is the main contribution to the handling overhead?
Tim can correct me if I'm wrong but I think the overhead was largely in the sort.
Thanks again for the patch. I'll get this checked in.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39402
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39402
-----------------------------------------------------------
Just curious: which part of the code is the main contribution to the handling overhead?
- Guozhang Wang
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39387
-----------------------------------------------------------
Ship it!
Ship It!
- Joel Koshy
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 4, 2014, 3:59 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 619-620
> > <https://reviews.apache.org/r/19957/diff/2/?file=547050#file547050line619>
> >
> > If this is expensive, perhaps we can only do that if we require all topics? If that makes the code too complicated, we don't have to do this optimization.
>
> Neha Narkhede wrote:
> Since this is invoked just once, it's not worth optimizing. Is it?
>
> Jun Rao wrote:
> We don't have to if it's too complicated. However, most requests will have a small # of topics in the request. In those cases, there is no need to make a copy of all cached topics.
The original code actually does a toSet of all the topics in each topic requested.
I just refactored metadata cache so it's organized by topic.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39542
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Neha Narkhede <ne...@gmail.com>.
> On April 4, 2014, 3:59 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 619-620
> > <https://reviews.apache.org/r/19957/diff/2/?file=547050#file547050line619>
> >
> > If this is expensive, perhaps we can only do that if we require all topics? If that makes the code too complicated, we don't have to do this optimization.
Since this is invoked just once, it's not worth optimizing. Is it?
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39542
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Jun Rao <ju...@gmail.com>.
> On April 4, 2014, 3:59 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, lines 619-620
> > <https://reviews.apache.org/r/19957/diff/2/?file=547050#file547050line619>
> >
> > If this is expensive, perhaps we can only do that if we require all topics? If that makes the code too complicated, we don't have to do this optimization.
>
> Neha Narkhede wrote:
> Since this is invoked just once, it's not worth optimizing. Is it?
We don't have to if it's too complicated. However, most requests will have a small # of topics in the request. In those cases, there is no need to make a copy of all cached topics.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39542
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39542
-----------------------------------------------------------
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/19957/#comment71948>
We actually don't need to hold the read lock for the part that auto-creates a topic, which can be a bit expensive.
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/19957/#comment71949>
If this is expensive, perhaps we can only do that if we require all topics? If that makes the code too complicated, we don't have to do this optimization.
- Jun Rao
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39546
-----------------------------------------------------------
core/src/main/scala/kafka/api/TopicMetadata.scala
<https://reviews.apache.org/r/19957/#comment71950>
This is a good change but can we just create an empty array of size numPartitions? Do we have to fill it?
- Neha Narkhede
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/
-----------------------------------------------------------
(Updated April 3, 2014, 1:39 a.m.)
Review request for kafka.
Bugs: KAFKA-1356
https://issues.apache.org/jira/browse/KAFKA-1356
Repository: kafka
Description
-------
KAFKA-1356 Improve topic metadata handling in kafka api
Diffs (updated)
-----
core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
Diff: https://reviews.apache.org/r/19957/diff/
Testing
-------
Thanks,
Timothy Chen
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 3, 2014, 1:13 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TopicMetadata.scala, line 35
> > <https://reviews.apache.org/r/19957/diff/1/?file=546093#file546093line35>
> >
> > Nitpick, but I think this would be a little neater:
> > (0 to numPartitions).map( <loop body> ).toArray
(0 to numPartitions).map(null).toArray actually gives me a null pointer exception :)
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39378
-----------------------------------------------------------
On April 2, 2014, 8:44 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 8:44 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Timothy Chen <tn...@apache.org>.
> On April 3, 2014, 1:13 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 638
> > <https://reviews.apache.org/r/19957/diff/1/?file=546094#file546094line638>
> >
> > I think this really should have been isr.map(aliveBrokers.get).filter(_.isEmpty)
this is passed to a function that only accepts Seq[Broker] and even if you filter the return type still is a Seq[Option[Broker]].
I think I'll just leave it as is
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39378
-----------------------------------------------------------
On April 2, 2014, 8:44 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 8:44 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Joel Koshy <jj...@gmail.com>.
> On April 3, 2014, 1:13 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TopicMetadata.scala, line 35
> > <https://reviews.apache.org/r/19957/diff/1/?file=546093#file546093line35>
> >
> > Nitpick, but I think this would be a little neater:
> > (0 to numPartitions).map( <loop body> ).toArray
>
> Timothy Chen wrote:
> (0 to numPartitions).map(null).toArray actually gives me a null pointer exception :)
Oh - I didn't mean map(null). I meant map { <loop body you currently have }.toArray
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39378
-----------------------------------------------------------
On April 3, 2014, 1:39 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 1:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/controller/KafkaController.scala fcabd0da201ccd478f5884bcca9b75a8c8ea8373
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 19957: Patch for KAFKA-1356
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19957/#review39378
-----------------------------------------------------------
core/src/main/scala/kafka/api/TopicMetadata.scala
<https://reviews.apache.org/r/19957/#comment71779>
Nitpick, but I think this would be a little neater:
(0 to numPartitions).map( <loop body> ).toArray
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/19957/#comment71771>
You can just call aliveBrokers.get(leader)
core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/19957/#comment71773>
I think this really should have been isr.map(aliveBrokers.get).filter(_.isEmpty)
- Joel Koshy
On April 2, 2014, 8:44 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19957/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 8:44 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1356
> https://issues.apache.org/jira/browse/KAFKA-1356
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1356 Improve topic metadata handling in kafka api
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/TopicMetadata.scala 0513a59ed94e556894b3515dc38666ee9a66ae3d
> core/src/main/scala/kafka/server/KafkaApis.scala c068ef69207c351eec413a595f1747c59f8b3983
>
> Diff: https://reviews.apache.org/r/19957/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Timothy Chen
>
>