You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Gwen Shapira <gs...@cloudera.com> on 2015/06/20 02:59:01 UTC
Review Request 35677: Patch for KAFKA-2288
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2288
https://issues.apache.org/jira/browse/KAFKA-2288
Repository: kafka
Description
-------
minor corrections to LogConfig and KafkaConfigTest
Diffs
-----
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
core/src/main/scala/kafka/server/KafkaServer.scala 52dc728bb1ab4b05e94dc528da1006040e2f28c9
core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
Diff: https://reviews.apache.org/r/35677/diff/
Testing
-------
Thanks,
Gwen Shapira
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Gwen Shapira <gs...@cloudera.com>.
> On June 22, 2015, 4:23 a.m., Jun Rao wrote:
> > Thanks for the patch. I created a topic w/o customized config. On broker startup, I still saw the following logging.
> >
> > [2015-06-21 21:01:21,569] INFO Read configuration for topic test (kafka.server.KafkaServer)
> > [2015-06-21 21:01:21,569] INFO LogConfig values:
> > segment.bytes = 1073741824
> > (kafka.log.LogConfig)
> >
> > In general, I am wondering if it will be too verbose to log even overriden configs when a broker has thousands of topics. Also, the user can always find out the overriden configs through the admin tool. The reason that we log the broker config is that there is no such tool.
You are right, it will be too much on large clusters.
I'll remove the extra logging.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review88723
-----------------------------------------------------------
On June 20, 2015, 12:59 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 12:59 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> minor corrections to LogConfig and KafkaConfigTest
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/main/scala/kafka/server/KafkaServer.scala 52dc728bb1ab4b05e94dc528da1006040e2f28c9
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review88723
-----------------------------------------------------------
Thanks for the patch. I created a topic w/o customized config. On broker startup, I still saw the following logging.
[2015-06-21 21:01:21,569] INFO Read configuration for topic test (kafka.server.KafkaServer)
[2015-06-21 21:01:21,569] INFO LogConfig values:
segment.bytes = 1073741824
(kafka.log.LogConfig)
In general, I am wondering if it will be too verbose to log even overriden configs when a broker has thousands of topics. Also, the user can always find out the overriden configs through the admin tool. The reason that we log the broker config is that there is no such tool.
- Jun Rao
On June 20, 2015, 12:59 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 12:59 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> minor corrections to LogConfig and KafkaConfigTest
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/main/scala/kafka/server/KafkaServer.scala 52dc728bb1ab4b05e94dc528da1006040e2f28c9
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Gwen Shapira <gs...@cloudera.com>.
> On July 7, 2015, 5:22 p.m., Jun Rao wrote:
> > Thanks for the patch. +1. You can commit after addressing the following minor comments.
I'm fixing these, but there was also some rebasing, so another look at the patch for sanity will be appreciated :)
> On July 7, 2015, 5:22 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala, lines 364-368
> > <https://reviews.apache.org/r/35677/diff/2/?file=989813#file989813line364>
> >
> > Would it be better to override equal() and hashcode() of KafkaConfig?
Overrode in AbstractConfig. I consider two AbstractConfig classes equal if the originals Map is equal. Sounds reasonable?
> On July 7, 2015, 5:22 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala, line 470
> > <https://reviews.apache.org/r/35677/diff/2/?file=989813#file989813line470>
> >
> > Is the ZK connect value correct?
Any string will be correct here, but I'll add a port too :)
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review90725
-----------------------------------------------------------
On June 22, 2015, 5:02 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:02 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removed logging of topic overrides
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review90725
-----------------------------------------------------------
Ship it!
Thanks for the patch. +1. You can commit after addressing the following minor comments.
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala (lines 364 - 368)
<https://reviews.apache.org/r/35677/#comment143860>
Would it be better to override equal() and hashcode() of KafkaConfig?
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala (line 470)
<https://reviews.apache.org/r/35677/#comment143861>
Is the ZK connect value correct?
- Jun Rao
On June 22, 2015, 5:02 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:02 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removed logging of topic overrides
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review94164
-----------------------------------------------------------
Ship it!
Looks good to me. +1. Just a minor comment below.
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala (lines 388 - 398)
<https://reviews.apache.org/r/35677/#comment148699>
Is this test really needed? KafkaConfig.fromProps() just calls the constructor of KafkaConfig.
- Jun Rao
On Aug. 4, 2015, 11:19 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 11:19 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rebased and added equals to AbstractConfig to make tests more concise
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java ec3ae15ec78fb82599543125917071e1f8485eb0
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 04a02e08a54139ee1a298c5354731bae009efef3
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala d3544526b66ce1f146f6986401299da9ebe49cd4
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/#review94166
-----------------------------------------------------------
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala (lines 388 - 398)
<https://reviews.apache.org/r/35677/#comment148700>
You are right, I don't see the point of this. I'll remove and commit.
- Gwen Shapira
On Aug. 4, 2015, 11:19 p.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35677/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 11:19 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2288
> https://issues.apache.org/jira/browse/KAFKA-2288
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rebased and added equals to AbstractConfig to make tests more concise
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java ec3ae15ec78fb82599543125917071e1f8485eb0
> core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 04a02e08a54139ee1a298c5354731bae009efef3
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala d3544526b66ce1f146f6986401299da9ebe49cd4
>
> Diff: https://reviews.apache.org/r/35677/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 11:19 p.m.)
Review request for kafka.
Bugs: KAFKA-2288
https://issues.apache.org/jira/browse/KAFKA-2288
Repository: kafka
Description (updated)
-------
rebased and added equals to AbstractConfig to make tests more concise
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java ec3ae15ec78fb82599543125917071e1f8485eb0
core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 04a02e08a54139ee1a298c5354731bae009efef3
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala d3544526b66ce1f146f6986401299da9ebe49cd4
Diff: https://reviews.apache.org/r/35677/diff/
Testing
-------
Thanks,
Gwen Shapira
Re: Review Request 35677: Patch for KAFKA-2288
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35677/
-----------------------------------------------------------
(Updated June 22, 2015, 5:02 p.m.)
Review request for kafka.
Bugs: KAFKA-2288
https://issues.apache.org/jira/browse/KAFKA-2288
Repository: kafka
Description (updated)
-------
removed logging of topic overrides
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java bae528d31516679bed88ee61b408f209f185a8cc
core/src/main/scala/kafka/log/LogConfig.scala fc41132d2bf29439225ec581829eb479f98cc416
core/src/test/scala/unit/kafka/log/LogConfigTest.scala 19dcb47f3f406b8d6c3668297450ab6b534e4471
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 2428dbd7197a58cf4cad42ef82b385dab3a2b15e
Diff: https://reviews.apache.org/r/35677/diff/
Testing
-------
Thanks,
Gwen Shapira