You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Timothy Chen <tn...@apache.org> on 2014/04/25 19:25:30 UTC
Review Request 20718: Patch for KAFKA-1384
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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 (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
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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 (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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20718/#review42016
-----------------------------------------------------------
core/src/main/scala/kafka/server/BrokerStates.scala
<https://reviews.apache.org/r/20718/#comment75750>
ShutDown => Shutdown?
core/src/main/scala/kafka/server/BrokerStates.scala
<https://reviews.apache.org/r/20718/#comment75749>
currentState needs to be volatile so that the JMX thread can read the latest value.
Also, could we add some comments on BrokerState? For example, what are the valid state transition paths?
- Jun Rao
On May 2, 2014, 1:47 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> 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
> -----
>
> 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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 20718: Patch for KAFKA-1384
Posted by Timothy Chen <tn...@apache.org>.
> 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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
>
>