You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/02 23:07:30 UTC

[GitHub] [kafka] cmccabe opened a new pull request #10028: Replace BrokerStates.scala with BrokerState.java

cmccabe opened a new pull request #10028:
URL: https://github.com/apache/kafka/pull/10028


   Making BrokerState a straightforward Java enum makes it easier to use from Java code.
   
   The existing BrokerState class is also awkward to use because it is mutable
   (it holds a volatile variable inside).  While there is also an immutable
   case class, there is no way to go from a numeric type to the case class.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] cmccabe merged pull request #10028: Replace BrokerStates.scala with BrokerState.java

Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10028:
URL: https://github.com/apache/kafka/pull/10028


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] cmccabe commented on a change in pull request #10028: Replace BrokerStates.scala with BrokerState.java

Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10028:
URL: https://github.com/apache/kafka/pull/10028#discussion_r569604728



##########
File path: core/src/main/scala/kafka/log/LogManager.scala
##########
@@ -60,7 +61,7 @@ class LogManager(logDirs: Seq[File],
                  val retentionCheckMs: Long,
                  val maxPidExpirationMs: Int,
                  scheduler: Scheduler,
-                 val brokerState: BrokerState,
+                 val brokerState: AtomicReference[BrokerState],

Review comment:
       The log manager needs to alter the broker state using the `AtomicReference`.  Since the broker state is owned by `KafkaBroker`, this would not be possible by just using a volatile variable.
   
   (This is an ugly pattern, I think, and a later patch set removes it.  But in the interest of keeping this PR small, we preserve the original behavior where LogManager changes the broker state directly for now)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #10028: Replace BrokerStates.scala with BrokerState.java

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10028:
URL: https://github.com/apache/kafka/pull/10028#discussion_r569239693



##########
File path: core/src/main/scala/kafka/log/LogManager.scala
##########
@@ -60,7 +61,7 @@ class LogManager(logDirs: Seq[File],
                  val retentionCheckMs: Long,
                  val maxPidExpirationMs: Int,
                  scheduler: Scheduler,
-                 val brokerState: BrokerState,
+                 val brokerState: AtomicReference[BrokerState],

Review comment:
       Is there a reason why you're using an AtomicReference instead of a volatile var?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] chia7712 commented on a change in pull request #10028: Replace BrokerStates.scala with BrokerState.java

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10028:
URL: https://github.com/apache/kafka/pull/10028#discussion_r569116805



##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java
##########
@@ -241,13 +240,13 @@ public String zKConnectString() {
     }
 
     /**
-     * Get the brokers that have a {@link RunningAsBroker} state.
+     * Get the brokers that have a {@link BrokerState.RUNNING} state.

Review comment:
       ```{@link BrokerState#RUNNING} ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org