kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aditya Auradkar" <aaurad...@linkedin.com>
Subject Re: Review Request 34554: Patch for KAFKA-2205
Date Thu, 02 Jul 2015 01:37:50 GMT


> On June 27, 2015, 2:48 p.m., Jun Rao wrote:
> > Thanks for the patch. A few comments below.
> > 
> > We plan to add the AlterConfig/DescribeConfig requests. Will that be handled in
a separate jira? Also, are the new requests format listed in the KIP?

Yeah, I'm planning to tackle the AlterConfig/DescribeConfig stuff in a separate jira. Those
requests are listed on KIP-4


> On June 27, 2015, 2:48 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala, lines 99-100
> > <https://reviews.apache.org/r/34554/diff/1/?file=966847#file966847line99>
> >
> >     Should we ignore it or should we error it like the rest?

Actually, I think we should ignore such notifications. The old-style notifications parsed
by TopicConfigManager aren't JSON. They simply have a string representing the topic name.
The new code can expect to receive these notifications (leftovers prior to the upgrade) and
shouldn't fail to process the other more recent notifications IMO.


> On June 27, 2015, 2:48 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ConfigHandler.scala, line 23
> > <https://reviews.apache.org/r/34554/diff/1/?file=966839#file966839line23>
> >
> >     Could we use groupBy{ case(key, value) => ...} instead of using tuples to
make it clear what we reference?

Using tuples now. I'm still using _ for the final map function because it seems quite clear
what is being referenced


> On June 27, 2015, 2:48 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/ConfigHandler.scala, lines 43-48
> > <https://reviews.apache.org/r/34554/diff/1/?file=966839#file966839line43>
> >
> >     We need to store the client configs in a map and provide an api to access them,
right?

I've added a map to store these configs. This is a no-op right now. I don't think an API is
necessary because we can use them within the same class i.e. quota changes can be done within
processConfigChanges.


- Aditya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34554/#review89623
-----------------------------------------------------------


On May 21, 2015, 6:32 p.m., Aditya Auradkar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34554/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 6:32 p.m.)
> 
> 
> Review request for kafka and Joel Koshy.
> 
> 
> Bugs: KAFKA-2205
>     https://issues.apache.org/jira/browse/KAFKA-2205
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-2205. Summary of changes:
> 
> 1. Generalized TopicConfigManager to DynamicConfigManager. It is now able to handle multiple
types of entities.
> 2. Changed format of the notification znode as described in KIP-21
> 3. Replaced TopicConfigManager with DynamicConfigManager.
> 4. Added new testcases. Existing testcases all pass
> 5. Added ConfigCommand to handle all config changes. Eventually this will make calls
to the broker once the new API's are built for now it speaks to ZK directly
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/admin/AdminUtils.scala f06edf41c732a7b794e496d0048b0ce6f897e72b

>   core/src/main/scala/kafka/admin/ConfigCommand.scala PRE-CREATION 
>   core/src/main/scala/kafka/admin/TopicCommand.scala 8e6f18633b25bf1beee3f813b28ef7aa7d779d7b

>   core/src/main/scala/kafka/cluster/Partition.scala 730a232482fdf77be5704cdf5941cfab3828db88

>   core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0

>   core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala 3b15ab4eef22c6f50a7483e99a6af40fb55aca9f

>   core/src/main/scala/kafka/controller/TopicDeletionManager.scala 64ecb499f24bc801d48f86e1612d927cc08e006d

>   core/src/main/scala/kafka/server/ConfigHandler.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaServer.scala ea6d165d8e5c3146d2c65e8ad1a513308334bf6f

>   core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3

>   core/src/main/scala/kafka/server/TopicConfigManager.scala b675a7e45ea4f4179f8b15fe221fd988aff13aa0

>   core/src/main/scala/kafka/utils/ZkUtils.scala 2618dd39b925b979ad6e4c0abd5c6eaafb3db5d5

>   core/src/test/scala/unit/kafka/admin/AdminTest.scala efb2f8e79b3faef78722774b951fea828cd50374

>   core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala c7136f20972614ac47aa57ab13e3c94ef775a4b7

>   core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 7877f6ca1845c2edbf96d4a9783a07a552db8f07

> 
> Diff: https://reviews.apache.org/r/34554/diff/
> 
> 
> Testing
> -------
> 
> 1. Added new testcases for new code.
> 2. Verified that both topic and client configs can be changed dynamically by starting
a local cluster
> 
> 
> Thanks,
> 
> Aditya Auradkar
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message