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/11 08:09:23 UTC
Review Request 35347: Patch for KAFKA-2249
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2249
https://issues.apache.org/jira/browse/KAFKA-2249
Repository: kafka
Description
-------
modified KafkaConfig to implement AbstractConfig. This resulted in somewhat cleaner code, and we preserve the original Properties for use by MetricReporter
Diffs
-----
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0
core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
Diff: https://reviews.apache.org/r/35347/diff/
Testing
-------
Thanks,
Gwen Shapira
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Gwen Shapira <gs...@cloudera.com>.
> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 964-965
> > <https://reviews.apache.org/r/35347/diff/1/?file=982452#file982452line964>
> >
> > Is this castable since props is a map? Also, toProps is really just used by LogConfig which converts Properties back to a map. Perhaps we don't even need this method and just let LogConfig access KafkaConfig.originals().
> >
> > KafkaConfig.getMetricClasses() uses toProps, but is never used. We can just remove that method.
> >
> > Also, perhaps it's useful and convenient to turn LogConfig into an AbstractConfig too.
>
> Gwen Shapira wrote:
> I agree that we don't need to expose toProps since we can let other classes access originals directly.
>
> Just note that the current design allows for users to pass arbitrary properties to reporters by placing them in server.properties (since we pass original properties along when configuring reporters in getMetricClasses) - this is a critical feature and why getMetricClasses used to call toProps and will now call originals.
Turning LogConfig into an AbstractConfig should have been its own JIRA. The patch is now quite large since I had to modify creation of LogConfig in a bunch of tests.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/#review87643
-----------------------------------------------------------
On June 18, 2015, 12:35 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35347/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 12:35 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2249
> https://issues.apache.org/jira/browse/KAFKA-2249
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Moved LogConfig to implement AbstractConfig too. This means modifying most Log tests, and some changes to defaults
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
> 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/log/LogConfig.scala f64fd79ee4cdd5ad15cd9b14fe7247464cde1e94
> core/src/main/scala/kafka/log/LogManager.scala e781ebac2677ebb22e0c1fef0cf7e5ad57c74ea4
> core/src/main/scala/kafka/server/KafkaApis.scala c7debe458ce9d80024b3f8544c92ebe3e14159dc
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 181cbc16b3780ffa77966cbc26337d2c39be9a72
> core/src/main/scala/kafka/server/TopicConfigManager.scala b675a7e45ea4f4179f8b15fe221fd988aff13aa0
> core/src/main/scala/kafka/utils/CoreUtils.scala d0a8fa701564b4c13b3cd6501e1b6218d77e8e06
> core/src/test/scala/other/kafka/StressTestLog.scala c0e248d669c7bd653f512af7f72d895c38772f83
> core/src/test/scala/other/kafka/TestLinearWriteSpeed.scala 3034c4f9b0d026e25ce045689d9a9f99a59a10ec
> core/src/test/scala/unit/kafka/log/BrokerCompressionTest.scala 375555f0684bbd0bfaf64b765ce04a928e257f0a
> core/src/test/scala/unit/kafka/log/CleanerTest.scala 8b8249a35322a60ca94cb385a6cad25943dd1cc9
> core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 471ddff9bff1bdfa277c071e59e5c6b749b9c74f
> core/src/test/scala/unit/kafka/log/LogConfigTest.scala 3fd5a53f9b0edc0a7a169a185cd3041ea1ae7658
> core/src/test/scala/unit/kafka/log/LogManagerTest.scala 01dfbc4f8d21f6905327cd4ed6c61d657adc0143
> core/src/test/scala/unit/kafka/log/LogTest.scala 8e095d652851f05365e1d3bbe3e9e1c3345b7a40
> core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 7877f6ca1845c2edbf96d4a9783a07a552db8f07
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala c487f361949b2ca2b6d1b5e2c7fb9ba83c8e53c1
>
> Diff: https://reviews.apache.org/r/35347/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Gwen Shapira <gs...@cloudera.com>.
> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 964-965
> > <https://reviews.apache.org/r/35347/diff/1/?file=982452#file982452line964>
> >
> > Is this castable since props is a map? Also, toProps is really just used by LogConfig which converts Properties back to a map. Perhaps we don't even need this method and just let LogConfig access KafkaConfig.originals().
> >
> > KafkaConfig.getMetricClasses() uses toProps, but is never used. We can just remove that method.
> >
> > Also, perhaps it's useful and convenient to turn LogConfig into an AbstractConfig too.
I agree that we don't need to expose toProps since we can let other classes access originals directly.
Just note that the current design allows for users to pass arbitrary properties to reporters by placing them in server.properties (since we pass original properties along when configuring reporters in getMetricClasses) - this is a critical feature and why getMetricClasses used to call toProps and will now call originals.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/#review87643
-----------------------------------------------------------
On June 11, 2015, 6:09 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35347/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:09 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2249
> https://issues.apache.org/jira/browse/KAFKA-2249
>
>
> Repository: kafka
>
>
> Description
> -------
>
> modified KafkaConfig to implement AbstractConfig. This resulted in somewhat cleaner code, and we preserve the original Properties for use by MetricReporter
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
> core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
>
> Diff: https://reviews.apache.org/r/35347/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Gwen Shapira <gs...@cloudera.com>.
> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 485
> > <https://reviews.apache.org/r/35347/diff/1/?file=982452#file982452line485>
> >
> > Is there a particular reason to change this to a long?
It is used as LONG everywhere in the code.
> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 492
> > <https://reviews.apache.org/r/35347/diff/1/?file=982452#file982452line492>
> >
> > Is there a particular reason to change this to a long?
It is used as LONG everywhere in the code.
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/#review87643
-----------------------------------------------------------
On June 11, 2015, 6:09 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35347/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:09 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2249
> https://issues.apache.org/jira/browse/KAFKA-2249
>
>
> Repository: kafka
>
>
> Description
> -------
>
> modified KafkaConfig to implement AbstractConfig. This resulted in somewhat cleaner code, and we preserve the original Properties for use by MetricReporter
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
> core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
>
> Diff: https://reviews.apache.org/r/35347/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/#review87643
-----------------------------------------------------------
Thanks a lot for cleaning this up! A few comments below.
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/35347/#comment140043>
Is there a particular reason to change this to a long?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/35347/#comment140044>
Is there a particular reason to change this to a long?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/35347/#comment140040>
Is this castable since props is a map? Also, toProps is really just used by LogConfig which converts Properties back to a map. Perhaps we don't even need this method and just let LogConfig access KafkaConfig.originals().
KafkaConfig.getMetricClasses() uses toProps, but is never used. We can just remove that method.
Also, perhaps it's useful and convenient to turn LogConfig into an AbstractConfig too.
- Jun Rao
On June 11, 2015, 6:09 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35347/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:09 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2249
> https://issues.apache.org/jira/browse/KAFKA-2249
>
>
> Repository: kafka
>
>
> Description
> -------
>
> modified KafkaConfig to implement AbstractConfig. This resulted in somewhat cleaner code, and we preserve the original Properties for use by MetricReporter
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
> core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
>
> Diff: https://reviews.apache.org/r/35347/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/#review87981
-----------------------------------------------------------
Also, could you add a main() method in KafkaConfig like that in ProducerConfig so that we can generate the config doc in html?
- Jun Rao
On June 11, 2015, 6:09 a.m., Gwen Shapira wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35347/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 6:09 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2249
> https://issues.apache.org/jira/browse/KAFKA-2249
>
>
> Repository: kafka
>
>
> Description
> -------
>
> modified KafkaConfig to implement AbstractConfig. This resulted in somewhat cleaner code, and we preserve the original Properties for use by MetricReporter
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
> core/src/main/scala/kafka/controller/KafkaController.scala 69bba243a9a511cc5292b43da0cc48e421a428b0
> core/src/main/scala/kafka/server/KafkaApis.scala d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd
> core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala b31b432a226ba79546dd22ef1d2acbb439c2e9a3
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala ace6321b36d809946554d205bc926c9c76a43bd6
>
> Diff: https://reviews.apache.org/r/35347/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gwen Shapira
>
>
Re: Review Request 35347: Patch for KAFKA-2249
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35347/
-----------------------------------------------------------
(Updated June 18, 2015, 12:35 a.m.)
Review request for kafka.
Bugs: KAFKA-2249
https://issues.apache.org/jira/browse/KAFKA-2249
Repository: kafka
Description (updated)
-------
Moved LogConfig to implement AbstractConfig too. This means modifying most Log tests, and some changes to defaults
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java c4fa058692f50abb4f47bd344119d805c60123f5
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/log/LogConfig.scala f64fd79ee4cdd5ad15cd9b14fe7247464cde1e94
core/src/main/scala/kafka/log/LogManager.scala e781ebac2677ebb22e0c1fef0cf7e5ad57c74ea4
core/src/main/scala/kafka/server/KafkaApis.scala c7debe458ce9d80024b3f8544c92ebe3e14159dc
core/src/main/scala/kafka/server/KafkaConfig.scala 2d75186a110075e0c322db4b9f7a8c964a7a3e88
core/src/main/scala/kafka/server/KafkaServer.scala b320ce9f6a12c0ee392e91beb82e8804d167f9f4
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 181cbc16b3780ffa77966cbc26337d2c39be9a72
core/src/main/scala/kafka/server/TopicConfigManager.scala b675a7e45ea4f4179f8b15fe221fd988aff13aa0
core/src/main/scala/kafka/utils/CoreUtils.scala d0a8fa701564b4c13b3cd6501e1b6218d77e8e06
core/src/test/scala/other/kafka/StressTestLog.scala c0e248d669c7bd653f512af7f72d895c38772f83
core/src/test/scala/other/kafka/TestLinearWriteSpeed.scala 3034c4f9b0d026e25ce045689d9a9f99a59a10ec
core/src/test/scala/unit/kafka/log/BrokerCompressionTest.scala 375555f0684bbd0bfaf64b765ce04a928e257f0a
core/src/test/scala/unit/kafka/log/CleanerTest.scala 8b8249a35322a60ca94cb385a6cad25943dd1cc9
core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 471ddff9bff1bdfa277c071e59e5c6b749b9c74f
core/src/test/scala/unit/kafka/log/LogConfigTest.scala 3fd5a53f9b0edc0a7a169a185cd3041ea1ae7658
core/src/test/scala/unit/kafka/log/LogManagerTest.scala 01dfbc4f8d21f6905327cd4ed6c61d657adc0143
core/src/test/scala/unit/kafka/log/LogTest.scala 8e095d652851f05365e1d3bbe3e9e1c3345b7a40
core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala 7877f6ca1845c2edbf96d4a9783a07a552db8f07
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala c487f361949b2ca2b6d1b5e2c7fb9ba83c8e53c1
Diff: https://reviews.apache.org/r/35347/diff/
Testing
-------
Thanks,
Gwen Shapira