Re: Review Request 20718: Patch for KAFKA-1384

2014-05-06 Thread Jun Rao 

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


The patch doesn't apply any more. Could you rebase?


core/src/main/scala/kafka/log/Log.scala
https://reviews.apache.org/r/20718/#comment76087

No need for this change.



core/src/main/scala/kafka/server/KafkaServerStartable.scala
https://reviews.apache.org/r/20718/#comment76088

Could you add a comment for the intent of this method?


- Jun Rao


On May 6, 2014, 12:15 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 6, 2014, 12:15 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
   core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
 be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
   core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
 a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
   core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
 41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-06 Thread Timothy Chen 

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

(Updated May 6, 2014, 8:21 p.m.)


Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
401bf1ea353d30353f9bae4a396302b15aa059c6 
  core/src/main/scala/kafka/log/LogManager.scala 
ab72cffd439c867051247e955dde2b6ade6836b8 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 
  core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
  core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
  core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Jun Rao 

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



core/src/main/scala/kafka/server/BrokerStates.scala
https://reviews.apache.org/r/20718/#comment75904

This diagram is very helpful. We need an arrow from RunningAsController to 
RendingControlledShutdown.


- Jun Rao


On May 2, 2014, 7:13 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 2, 2014, 7:13 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Timothy Chen 

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

(Updated May 5, 2014, 6:05 p.m.)


Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
933de9dd324c7086efe6aa610335ef370d9e9c12 
  core/src/main/scala/kafka/log/Log.scala 
b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
  core/src/main/scala/kafka/log/LogManager.scala 
ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Jun Rao 

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


Compilation error when trying to run the unit tests.

kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:35:
 not enough arguments for constructor LogManager: (logDirs: 
Array[java.io.File],topicConfigs: 
scala.collection.Map[String,kafka.log.LogConfig],defaultConfig: 
kafka.log.LogConfig,cleanerConfig: kafka.log.CleanerConfig,flushCheckMs: 
Long,flushCheckpointMs: Long,retentionCheckMs: Long,scheduler: 
kafka.utils.Scheduler,brokerState: kafka.server.BrokerState,time: 
kafka.utils.Time)kafka.log.LogManager.
Unspecified value parameter brokerState.
  val logManagers = configs.map(config = new LogManager(logDirs = 
config.logDirs.map(new File(_)).toArray,
  ^
/Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:47:
 value logDirs is not a member of Nothing
for(manager - logManagers; dir - manager.logDirs)
   ^
/Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:67:
 value createLog is not a member of Nothing
val log0 = logManagers(0).createLog(TopicAndPartition(topic, 0), 
LogConfig())
  ^
/Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:106:
 value createLog is not a member of Nothing
val topic1Log0 = logManagers(0).createLog(TopicAndPartition(topic1, 0), 
LogConfig())
^
/Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:122:
 value createLog is not a member of Nothing
val topic2Log0 = logManagers(0).createLog(TopicAndPartition(topic2, 0), 
LogConfig())
^
/Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:64:
 not enough arguments for constructor LogManager: (logDirs: 
Array[java.io.File],topicConfigs: 
scala.collection.Map[String,kafka.log.LogConfig],defaultConfig: 
kafka.log.LogConfig,cleanerConfig: kafka.log.CleanerConfig,flushCheckMs: 
Long,flushCheckpointMs: Long,retentionCheckMs: Long,scheduler: 
kafka.utils.Scheduler,brokerState: kafka.server.BrokerState,time: 
kafka.utils.Time)kafka.log.LogManager.
Unspecified value parameter brokerState.
return new LogManager(logDirs,


- Jun Rao


On May 5, 2014, 6:05 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 5, 2014, 6:05 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Timothy Chen 


 On May 5, 2014, 8:51 p.m., Jun Rao wrote:
  Compilation error when trying to run the unit tests.
  
  kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:35:
   not enough arguments for constructor LogManager: (logDirs: 
  Array[java.io.File],topicConfigs: 
  scala.collection.Map[String,kafka.log.LogConfig],defaultConfig: 
  kafka.log.LogConfig,cleanerConfig: kafka.log.CleanerConfig,flushCheckMs: 
  Long,flushCheckpointMs: Long,retentionCheckMs: Long,scheduler: 
  kafka.utils.Scheduler,brokerState: kafka.server.BrokerState,time: 
  kafka.utils.Time)kafka.log.LogManager.
  Unspecified value parameter brokerState.
val logManagers = configs.map(config = new LogManager(logDirs = 
  config.logDirs.map(new File(_)).toArray,
^
  /Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:47:
   value logDirs is not a member of Nothing
  for(manager - logManagers; dir - manager.logDirs)
 ^
  /Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:67:
   value createLog is not a member of Nothing
  val log0 = logManagers(0).createLog(TopicAndPartition(topic, 0), 
  LogConfig())
^
  /Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:106:
   value createLog is not a member of Nothing
  val topic1Log0 = logManagers(0).createLog(TopicAndPartition(topic1, 0), 
  LogConfig())
  ^
  /Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala:122:
   value createLog is not a member of Nothing
  val topic2Log0 = logManagers(0).createLog(TopicAndPartition(topic2, 0), 
  LogConfig())
  ^
  /Users/jrao/Intellij/kafka_gradle/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:64:
   not enough arguments for constructor LogManager: (logDirs: 
  Array[java.io.File],topicConfigs: 
  scala.collection.Map[String,kafka.log.LogConfig],defaultConfig: 
  kafka.log.LogConfig,cleanerConfig: kafka.log.CleanerConfig,flushCheckMs: 
  Long,flushCheckpointMs: Long,retentionCheckMs: Long,scheduler: 
  kafka.utils.Scheduler,brokerState: kafka.server.BrokerState,time: 
  kafka.utils.Time)kafka.log.LogManager.
  Unspecified value parameter brokerState.
  return new LogManager(logDirs,
 

Ah sorry, oddly I was able to run it but didn't try out all the tests. Fixed it 
in the next patch.


- Timothy


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


On May 5, 2014, 9:25 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 5, 2014, 9:25 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
   core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
 be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
   core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
 a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
   core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
 41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Jun Rao 

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



core/src/test/scala/unit/kafka/log/LogManagerTest.scala
https://reviews.apache.org/r/20718/#comment76008

Could we explicitly specify all parameters as we do in 
testRecoveryDirectoryMappingWithTrailingSlash()?



core/src/test/scala/unit/kafka/log/LogManagerTest.scala
https://reviews.apache.org/r/20718/#comment76009

Could we explicitly specify all parameters as we do in 
testRecoveryDirectoryMappingWithTrailingSlash()?



core/src/test/scala/unit/kafka/log/LogManagerTest.scala
https://reviews.apache.org/r/20718/#comment76010

Could we explicitly specify all parameters as we do in 
testRecoveryDirectoryMappingWithTrailingSlash()?


- Jun Rao


On May 5, 2014, 9:25 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 5, 2014, 9:25 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
   core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
 be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
   core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
 a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
   core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
 41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Timothy Chen 

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

(Updated May 6, 2014, 12:15 a.m.)


Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
933de9dd324c7086efe6aa610335ef370d9e9c12 
  core/src/main/scala/kafka/log/Log.scala 
b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
  core/src/main/scala/kafka/log/LogManager.scala 
ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 
  core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
  core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
  core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-05 Thread Joel Koshy 

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

Ship it!


Minor comment, mentioned earlier: prefer just State instead of BrokerState.

- Joel Koshy


On May 6, 2014, 12:15 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated May 6, 2014, 12:15 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
   core/src/test/scala/unit/kafka/log/LogManagerTest.scala 
 be1a1ee3ac5c1dd8d658deabcfc740e88e053288 
   core/src/test/scala/unit/kafka/server/HighwatermarkPersistenceTest.scala 
 a78f7cfa025b0c5d968381b6d5dc7aec894a612b 
   core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala 
 41ebc7acae2efdd8a58964aa746c90fa71ac9ba4 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-01 Thread Jun Rao 

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



core/src/main/scala/kafka/server/BrokerStates.scala
https://reviews.apache.org/r/20718/#comment75723

In scala, we have been using case classes as enum. See PartitionState.

Also, could PendingShutdownFromController be named 
PendingControlledShutdown?



core/src/main/scala/kafka/server/KafkaServer.scala
https://reviews.apache.org/r/20718/#comment75718

No need to prepend with the brokerId since the caller will know the broker 
that it's poking.



core/src/main/scala/kafka/server/KafkaServer.scala
https://reviews.apache.org/r/20718/#comment75719

Will that override the state to broker even though it's set to controller?



core/src/main/scala/kafka/server/KafkaServerStartable.scala
https://reviews.apache.org/r/20718/#comment75720

What's the intention of this method?


- Jun Rao


On April 26, 2014, 7:09 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated April 26, 2014, 7:09 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 f20c232e2daa63a10d91b965af52801af656477c 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-01 Thread Timothy Chen 


 On May 2, 2014, 1:09 a.m., Jun Rao wrote:
  core/src/main/scala/kafka/server/KafkaServer.scala, lines 108-109
  https://reviews.apache.org/r/20718/diff/2/?file=569132#file569132line108
 
  Will that override the state to broker even though it's set to 
  controller?

The controller only sets the state when it is elected, and from the server 
startup steps it will set as broker state first, and once the controller starts 
up and the elect happens then it will be set to controller state.


 On May 2, 2014, 1:09 a.m., Jun Rao wrote:
  core/src/main/scala/kafka/server/KafkaServerStartable.scala, lines 55-58
  https://reviews.apache.org/r/20718/diff/2/?file=569133#file569133line55
 
  What's the intention of this method?

This is to allow custom state that is not defined in the enums. One example is 
in our custom kafka server startable we can inject more states that is specific 
to different use cases.


- Timothy


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


On April 26, 2014, 7:09 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated April 26, 2014, 7:09 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 f20c232e2daa63a10d91b965af52801af656477c 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-05-01 Thread Timothy Chen 

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

(Updated May 2, 2014, 1:47 a.m.)


Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
933de9dd324c7086efe6aa610335ef370d9e9c12 
  core/src/main/scala/kafka/log/Log.scala 
b7bc5ffdecba7221b3d2e4bca2b0c703bc74fa12 
  core/src/main/scala/kafka/log/LogManager.scala 
ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-04-26 Thread Timothy Chen 


 On April 25, 2014, 6:35 p.m., Joel Koshy wrote:
  core/src/main/scala/kafka/server/KafkaServerStartable.scala, line 55
  https://reviews.apache.org/r/20718/diff/1/?file=568568#file568568line55
 
  Unused

This is actually exposed setting custom state that is not one of the defined 
enums. One example is for our internal custom kafka server startable override 
can allow more states that is not defined in BrokerStates. The alternative is 
to just come up with all possible enums and let that be the only option 
available, but I was thinking that might be a bit too limiting. I'm not sure if 
this approach is too permissive either, you have more thoughts?


 On April 25, 2014, 6:35 p.m., Joel Koshy wrote:
  core/src/main/scala/kafka/server/BrokerStates.scala, line 28
  https://reviews.apache.org/r/20718/diff/1/?file=568566#file568566line28
 
  Just a thought: I'm wondering if it is better to have a bit-vector 
  approach for states; although that will limit the number of possible states 
  - but I think that is fine.
  
  The main reason for this is that it will enable composing simultaneous 
  states. E.g., in this approach you cannot distinguish state 5 from state 3 
  (if a shutting down broker is the controller). Although we can probably 
  infer that from the fact that the other brokers are likely in state (2) or 
  by looking at the active controller count separately. Still, we currently 
  allow more than one broker to shut down.
  
  It also _might_ help catch erroneous dual states (due to bugs).
  
  What you have is probably fine for lifecycle states (except for the 
  above caveat). However, if we ever want to allow more-than-lifecycle states 
  (e.g., under-replicated is a state we _might_ want to include on this - 
  even though we have a separate URP mbean and it's not a lifecycle state; 
  another example is loading consumer offsets).

Erroneous dual states is something I thought of, especially when state changes 
rapidly it will be harder to detect.
The down side of having a bit field is also that you need to then also have to 
unset that state when you're going into another lifecycle state.

I think coming back to the high level of what we want for broker states, since 
really a broker should really just be in a state at one point of time, setting 
a particular state I think makes more sense.

Composite states probably makes more sense for different component states. 


- Timothy


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


On April 25, 2014, 5:25 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated April 25, 2014, 5:25 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 46df8d99d977a3b010a9b9f4698187fa9bfb2498 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-04-26 Thread Timothy Chen 

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

(Updated April 26, 2014, 7:09 a.m.)


Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
933de9dd324c7086efe6aa610335ef370d9e9c12 
  core/src/main/scala/kafka/log/Log.scala 
f20c232e2daa63a10d91b965af52801af656477c 
  core/src/main/scala/kafka/log/LogManager.scala 
ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Review Request 20718: Patch for KAFKA-1384

2014-04-25 Thread Timothy Chen 

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

Review request for kafka.


Bugs: KAFKA-1384
https://issues.apache.org/jira/browse/KAFKA-1384


Repository: kafka


Description
---

KAFKA-1384: Logging kafka state metric


Diffs
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
933de9dd324c7086efe6aa610335ef370d9e9c12 
  core/src/main/scala/kafka/log/Log.scala 
46df8d99d977a3b010a9b9f4698187fa9bfb2498 
  core/src/main/scala/kafka/log/LogManager.scala 
ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
  core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaServer.scala 
c208f83bed7fb91f07fae42f2b66892e6d46fecc 
  core/src/main/scala/kafka/server/KafkaServerStartable.scala 
acda52b801714bcc182edc0ced925f0e4b493fc1 

Diff: https://reviews.apache.org/r/20718/diff/


Testing
---


Thanks,

Timothy Chen

Re: Review Request 20718: Patch for KAFKA-1384

2014-04-25 Thread Joel Koshy 

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



core/src/main/scala/kafka/server/BrokerStates.scala
https://reviews.apache.org/r/20718/#comment74944

Just a thought: I'm wondering if it is better to have a bit-vector approach 
for states; although that will limit the number of possible states - but I 
think that is fine.

The main reason for this is that it will enable composing simultaneous 
states. E.g., in this approach you cannot distinguish state 5 from state 3 (if 
a shutting down broker is the controller). Although we can probably infer that 
from the fact that the other brokers are likely in state (2) or by looking at 
the active controller count separately. Still, we currently allow more than one 
broker to shut down.

It also _might_ help catch erroneous dual states (due to bugs).

What you have is probably fine for lifecycle states (except for the above 
caveat). However, if we ever want to allow more-than-lifecycle states (e.g., 
under-replicated is a state we _might_ want to include on this - even though we 
have a separate URP mbean and it's not a lifecycle state; another example is 
loading consumer offsets).



core/src/main/scala/kafka/server/KafkaServer.scala
https://reviews.apache.org/r/20718/#comment74947

I think we can drop CurrentBroker. Just -State (because it is shorter, 
as meaningful, and also because brokerstate does not fully fit with 
controllerstate)



core/src/main/scala/kafka/server/KafkaServerStartable.scala
https://reviews.apache.org/r/20718/#comment74959

Unused


- Joel Koshy


On April 25, 2014, 5:25 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/20718/
 ---
 
 (Updated April 25, 2014, 5:25 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-1384
 https://issues.apache.org/jira/browse/KAFKA-1384
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-1384: Logging kafka state metric
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/controller/KafkaController.scala 
 933de9dd324c7086efe6aa610335ef370d9e9c12 
   core/src/main/scala/kafka/log/Log.scala 
 46df8d99d977a3b010a9b9f4698187fa9bfb2498 
   core/src/main/scala/kafka/log/LogManager.scala 
 ac67f081e6219fd2181479e7a2bb88ea6044e6cc 
   core/src/main/scala/kafka/server/BrokerStates.scala PRE-CREATION 
   core/src/main/scala/kafka/server/KafkaServer.scala 
 c208f83bed7fb91f07fae42f2b66892e6d46fecc 
   core/src/main/scala/kafka/server/KafkaServerStartable.scala 
 acda52b801714bcc182edc0ced925f0e4b493fc1 
 
 Diff: https://reviews.apache.org/r/20718/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Timothy Chen