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/06/08 11:42:48 UTC

[GitHub] [kafka] mdedetrich opened a new pull request #10839: KAFKA-12913: Make case class's final

mdedetrich opened a new pull request #10839:
URL: https://github.com/apache/kafka/pull/10839


   This PR makes all of the Scala's `case class`'s final to ensure correctness of Kafka's Scala code that uses these case classes. In Scala its best practice to make `case class`'s final since `case class` automatically generates critical methods such as `hashcode`/`equals`/`unapply` which can break code if user's override these methods by subclassing.
   
   Please see the [KIP](https://cwiki.apache.org/confluence/display/KAFKA/KIP-754%3A+Make+Scala+case+class%27s+final) for more info.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/test/scala/integration/kafka/server/MultipleListenersWithSameSecurityProtocolBaseTest.scala
##########
@@ -180,7 +180,7 @@ abstract class MultipleListenersWithSameSecurityProtocolBaseTest extends ZooKeep
     props.put(s"${prefix}${KafkaConfig.SaslJaasConfigProp}", jaasConfig)
   }
 
-  case class ClientMetadata(val listenerName: ListenerName, val saslMechanism: String, topic: String) {

Review comment:
       `val` is unnecessary/pointless here




-- 
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] jlprat commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -178,9 +177,3 @@ class TestableDigestLoginModule extends DigestLoginModule {
     }
   }
 }
-
-class TestableJaasSection(contextName: String, modules: Seq[JaasModule]) extends JaasSection(contextName, modules) {

Review comment:
       So you removed this class, and no change on the test was needed?

##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -466,3 +459,10 @@ object TestRaftServer extends Logging {
   }
 
 }
+
+private[tools] sealed trait RaftEvent
+private[tools] final case class HandleClaim(epoch: Int) extends RaftEvent
+private[tools] case object HandleResign extends RaftEvent
+private[tools] final case class HandleCommit(reader: BatchReader[Array[Byte]]) extends RaftEvent
+private[tools] final case class HandleSnapshot(reader: SnapshotReader[Array[Byte]]) extends RaftEvent
+private[tools] case object Shutdown extends RaftEvent

Review comment:
       Any reason to move it down the file?

##########
File path: core/src/main/scala/kafka/server/ZkAdminManager.scala
##########
@@ -1143,3 +1141,5 @@ class ZkAdminManager(val config: KafkaConfig,
     retval
   }
 }
+
+private[server] final case class RequestStatus(user: String, mechanism: Option[ScramMechanism], legalRequest: Boolean, iterations: Int)

Review comment:
       Nice catch. Wondering why it was creating using lower case on the first place as it doesn't follow any Scala convention.

##########
File path: core/src/test/scala/integration/kafka/server/MultipleListenersWithSameSecurityProtocolBaseTest.scala
##########
@@ -180,7 +180,7 @@ abstract class MultipleListenersWithSameSecurityProtocolBaseTest extends ZooKeep
     props.put(s"${prefix}${KafkaConfig.SaslJaasConfigProp}", jaasConfig)
   }
 
-  case class ClientMetadata(val listenerName: ListenerName, val saslMechanism: String, topic: String) {

Review comment:
       Nice catch.

##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -69,7 +68,7 @@ class AclAuthorizerWithZkSaslTest extends ZooKeeperTestHarness with SaslSetup {
     val jaasSections = JaasTestUtils.zkSections
     val serverJaas = jaasSections.filter(_.contextName == "Server")
     val clientJaas = jaasSections.filter(_.contextName == "Client")
-      .map(section => new TestableJaasSection(section.contextName, section.modules))
+      .map(section => JaasSection(section.contextName, section.modules))

Review comment:
       Nice catch

##########
File path: core/src/main/scala/kafka/server/MetadataCache.scala
##########
@@ -462,10 +462,10 @@ class ZkMetadataCache(brokerId: Int) extends MetadataCache with Logging {
     }
   }
 
-  case class MetadataSnapshot(partitionStates: mutable.AnyRefMap[String, mutable.LongMap[UpdateMetadataPartitionState]],
-                              topicIds: Map[String, Uuid],
-                              controllerId: Option[Int],
-                              aliveBrokers: mutable.LongMap[Broker],
-                              aliveNodes: mutable.LongMap[collection.Map[ListenerName, Node]])
-
 }
+
+private[server] final case class MetadataSnapshot(partitionStates: mutable.AnyRefMap[String, mutable.LongMap[UpdateMetadataPartitionState]],

Review comment:
       I guess if there are no usages of it, it's fine to mark it as package private.

##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -178,9 +177,3 @@ class TestableDigestLoginModule extends DigestLoginModule {
     }
   }
 }
-
-class TestableJaasSection(contextName: String, modules: Seq[JaasModule]) extends JaasSection(contextName, modules) {

Review comment:
       Gotcha




-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -178,9 +177,3 @@ class TestableDigestLoginModule extends DigestLoginModule {
     }
   }
 }
-
-class TestableJaasSection(contextName: String, modules: Seq[JaasModule]) extends JaasSection(contextName, modules) {

Review comment:
       The only case in the entirety of Kafka where we subclass a `case class`. Since this is in tests and its only use was to better pretty print via `toString`, I have removed it since due to it being superficial.




-- 
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] mdedetrich edited a comment on pull request #10839: KAFKA-12913: Make case class's final

Posted by GitBox <gi...@apache.org>.
mdedetrich edited a comment on pull request #10839:
URL: https://github.com/apache/kafka/pull/10839#issuecomment-910295551


   I have just rebased the PR so there should be no conflicts, also removed the unnecessary migration changes in `upgrade.html`
   
   @mjsax @ijuma The PR is ready for review


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

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

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

[GitHub] [kafka] mdedetrich commented on pull request #10839: KAFKA-12913: Make case class's final

Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #10839:
URL: https://github.com/apache/kafka/pull/10839#issuecomment-858409809


   PR ready for review


-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/main/scala/kafka/server/ZkAdminManager.scala
##########
@@ -1143,3 +1141,5 @@ class ZkAdminManager(val config: KafkaConfig,
     retval
   }
 }
+
+private[server] final case class RequestStatus(user: String, mechanism: Option[ScramMechanism], legalRequest: Boolean, iterations: Int)

Review comment:
       Note that I have renamed `requestStatus` to `RequestStatus` (classes should start with a capital) and made it private since `RequestStatus` is only ever used internally.




-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -69,7 +68,7 @@ class AclAuthorizerWithZkSaslTest extends ZooKeeperTestHarness with SaslSetup {
     val jaasSections = JaasTestUtils.zkSections
     val serverJaas = jaasSections.filter(_.contextName == "Server")
     val clientJaas = jaasSections.filter(_.contextName == "Client")
-      .map(section => new TestableJaasSection(section.contextName, section.modules))
+      .map(section => JaasSection(section.contextName, section.modules))

Review comment:
       `new` here is pointless




-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/main/scala/kafka/server/MetadataCache.scala
##########
@@ -462,10 +462,10 @@ class ZkMetadataCache(brokerId: Int) extends MetadataCache with Logging {
     }
   }
 
-  case class MetadataSnapshot(partitionStates: mutable.AnyRefMap[String, mutable.LongMap[UpdateMetadataPartitionState]],
-                              topicIds: Map[String, Uuid],
-                              controllerId: Option[Int],
-                              aliveBrokers: mutable.LongMap[Broker],
-                              aliveNodes: mutable.LongMap[collection.Map[ListenerName, Node]])
-
 }
+
+private[server] final case class MetadataSnapshot(partitionStates: mutable.AnyRefMap[String, mutable.LongMap[UpdateMetadataPartitionState]],

Review comment:
       Note that I have made this `case class` private since its only ever used internally.




-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: gradle/spotbugs-exclude.xml
##########
@@ -132,6 +132,13 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
         <Bug pattern="REC_CATCH_EXCEPTION"/>
     </Match>
 
+    <Match>

Review comment:
       A spurious false warning that was being created by spotbugs




-- 
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] mdedetrich commented on pull request #10839: KAFKA-12913: Make case class's final

Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #10839:
URL: https://github.com/apache/kafka/pull/10839#issuecomment-865776830


   @mjsax @vvcephei @guozhangwang Sorry for directly pinging you guys but this PR has been sitting here for 2 weeks so I was wondering if you guys could look/comment on it (apparently you are also the Kafka committers that can understand Scala code)


-- 
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] mdedetrich commented on a change in pull request #10839: KAFKA-12913: Make case class's final

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



##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -466,3 +459,10 @@ object TestRaftServer extends Logging {
   }
 
 }
+
+private[tools] sealed trait RaftEvent
+private[tools] final case class HandleClaim(epoch: Int) extends RaftEvent
+private[tools] case object HandleResign extends RaftEvent
+private[tools] final case class HandleCommit(reader: BatchReader[Array[Byte]]) extends RaftEvent
+private[tools] final case class HandleSnapshot(reader: SnapshotReader[Array[Byte]]) extends RaftEvent
+private[tools] case object Shutdown extends RaftEvent

Review comment:
       Yes, I had to move the case classes either into an `object` or as a top level class because otherwise the Scala compiler warns about an unambigious reference (which fails to build in Kafka since Kafka doesn't allow warnings to come through). 

##########
File path: core/src/test/scala/unit/kafka/security/authorizer/AclAuthorizerWithZkSaslTest.scala
##########
@@ -178,9 +177,3 @@ class TestableDigestLoginModule extends DigestLoginModule {
     }
   }
 }
-
-class TestableJaasSection(contextName: String, modules: Seq[JaasModule]) extends JaasSection(contextName, modules) {

Review comment:
       There is a change which you commented on where `new TestableJaasSection` was changed to `JassSection`




-- 
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] mdedetrich commented on pull request #10839: KAFKA-12913: Make case class's final

Posted by GitBox <gi...@apache.org>.
mdedetrich commented on pull request #10839:
URL: https://github.com/apache/kafka/pull/10839#issuecomment-910295551


   I have just rebased the PR so there should be no conflicts, also removed the unnecessary migration changes in `upgrade.html`


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

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

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