Re: Review Request 35347: Patch for KAFKA-2249

2015-06-17 Thread Gwen Shapira 


> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 964-965
> > 
> >
> > 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 
> 37f0684bbd0bfaf64b765ce04a928e257f0a 
>   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

2015-06-17 Thread Gwen Shapira 

---
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 
37f0684bbd0bfaf64b765ce04a928e257f0a 
  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

2015-06-17 Thread Gwen Shapira 


> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 964-965
> > 
> >
> > 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

2015-06-15 Thread Jun Rao 

---
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

2015-06-11 Thread Gwen Shapira 


> On June 11, 2015, 11:22 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 485
> > 
> >
> > 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
> > 
> >
> > 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

2015-06-11 Thread Jun Rao 

---
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


Is there a particular reason to change this to a long?



core/src/main/scala/kafka/server/KafkaConfig.scala


Is there a particular reason to change this to a long?



core/src/main/scala/kafka/server/KafkaConfig.scala


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
> 
>