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 2020/09/17 15:22:17 UTC
[GitHub] [kafka] ijuma opened a new pull request #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
ijuma opened a new pull request #9299:
URL: https://github.com/apache/kafka/pull/9299
`foreachKv` invokes `foreachEntry` in Scala 2.13 and falls back to
`foreach` in Scala 2.12.
This change requires a newer version of scala-collection-compat, so
update it to the latest version (2.2.0).
### 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] ijuma merged pull request #9299: MINOR: Use `Map.forKeyValue` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #9299:
URL: https://github.com/apache/kafka/pull/9299
----------------------------------------------------------------
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 merged pull request #9299: MINOR: Use `Map.forKeyValue` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #9299:
URL: https://github.com/apache/kafka/pull/9299
----------------------------------------------------------------
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] jsancio commented on a change in pull request #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r491151232
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
`forKeyValue` LGTM since `for` is the language's keyword for `foreach` or `map`.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492361262
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Checked with @hachikuji and he's fine with this change.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r491163592
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
That was the motivation indeed. Great, I'll change it.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492077814
##########
File path: core/src/main/scala/kafka/admin/ZkSecurityMigrator.scala
##########
@@ -128,7 +129,7 @@ object ZkSecurityMigrator extends Logging {
// Now override any set system properties with explicitly-provided values from the config file
// Emit INFO logs due to camel-case property names encouraging mistakes -- help people see mistakes they make
info(s"Found ${zkTlsConfigFileProps.size()} ZooKeeper client configuration properties in file $filename")
- zkTlsConfigFileProps.asScala.foreach { case (key, value) =>
+ zkTlsConfigFileProps.asScala.forKeyValue { (key, value) =>
Review comment:
See https://github.com/apache/kafka/commit/847ff8f55735568fe879d490af297e047b994f5e for an explanation why `asScala` still makes sense when dealing with `Properties`.
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Checked with @hachikuji and he's fine with this change.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492025863
##########
File path: core/src/main/scala/kafka/admin/ZkSecurityMigrator.scala
##########
@@ -128,7 +129,7 @@ object ZkSecurityMigrator extends Logging {
// Now override any set system properties with explicitly-provided values from the config file
// Emit INFO logs due to camel-case property names encouraging mistakes -- help people see mistakes they make
info(s"Found ${zkTlsConfigFileProps.size()} ZooKeeper client configuration properties in file $filename")
- zkTlsConfigFileProps.asScala.foreach { case (key, value) =>
+ zkTlsConfigFileProps.asScala.forKeyValue { (key, value) =>
Review comment:
Could we replace such ```asScala.forKeyValue``` by java ```Map.forEach``` ? We don't need to convert it to scala collection actually.
----------------------------------------------------------------
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] dajac commented on pull request #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#issuecomment-695298146
'forKeyValue' sounds good to me as well.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492025863
##########
File path: core/src/main/scala/kafka/admin/ZkSecurityMigrator.scala
##########
@@ -128,7 +129,7 @@ object ZkSecurityMigrator extends Logging {
// Now override any set system properties with explicitly-provided values from the config file
// Emit INFO logs due to camel-case property names encouraging mistakes -- help people see mistakes they make
info(s"Found ${zkTlsConfigFileProps.size()} ZooKeeper client configuration properties in file $filename")
- zkTlsConfigFileProps.asScala.foreach { case (key, value) =>
+ zkTlsConfigFileProps.asScala.forKeyValue { (key, value) =>
Review comment:
Could we replace such ```asScala.forKeyValue``` by java ```Map.forEach``` ? We don't need to convert it to scala collection actually.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r491149647
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Thanks for the feedback. I agree that the name could be improved. I was hoping for something concise and descriptive. `foreachPair` is not bad, but you'd expect to get a pair (tuple) and this is to avoid that. How about `forKeyValue`?
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492361262
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Checked with @hachikuji and he's fine with this change.
----------------------------------------------------------------
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] hachikuji commented on a change in pull request #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r490586608
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
The name reads a tad awkwardly. I wonder if `foreachKeyValue` would be too verbose. Or maybe `foreachMapEntry`?
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r492077814
##########
File path: core/src/main/scala/kafka/admin/ZkSecurityMigrator.scala
##########
@@ -128,7 +129,7 @@ object ZkSecurityMigrator extends Logging {
// Now override any set system properties with explicitly-provided values from the config file
// Emit INFO logs due to camel-case property names encouraging mistakes -- help people see mistakes they make
info(s"Found ${zkTlsConfigFileProps.size()} ZooKeeper client configuration properties in file $filename")
- zkTlsConfigFileProps.asScala.foreach { case (key, value) =>
+ zkTlsConfigFileProps.asScala.forKeyValue { (key, value) =>
Review comment:
See https://github.com/apache/kafka/commit/847ff8f55735568fe879d490af297e047b994f5e for an explanation why `asScala` still makes sense when dealing with `Properties`.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r491149798
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Also, note that this will go away once we drop support for Scala 2.12.
----------------------------------------------------------------
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 #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r490344276
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -133,7 +133,7 @@ object GetOffsetShell {
}
}
- partitionOffsets.toSeq.sortBy { case (tp, _) => tp.partition }.foreach { case (tp, offset) =>
+ partitionOffsets.toArray.sortBy { case (tp, _) => tp.partition }.foreach { case (tp, offset) =>
Review comment:
Unrelated clean-up I noticed.
----------------------------------------------------------------
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] jsancio commented on a change in pull request #9299: MINOR: Use `Map.foreachKv` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #9299:
URL: https://github.com/apache/kafka/pull/9299#discussion_r491078577
##########
File path: core/src/main/scala/kafka/utils/Implicits.scala
##########
@@ -46,4 +47,21 @@ object Implicits {
}
+ /**
+ * Exposes `foreachKv` which maps to `foreachEntry` in Scala 2.13 and `foreach` in Scala 2.12
+ * (with the help of scala.collection.compat). `foreachEntry` avoids the tuple allocation and
+ * is more efficient.
+ *
+ * This was not named `foreachEntry` to avoid `unused import` warnings in Scala 2.13 (the implicit
+ * would not be triggered in Scala 2.13 since `Map.foreachEntry` would have precedence).
+ */
+ @nowarn("cat=unused-imports")
+ implicit class MapExtensionMethods[K, V](private val self: scala.collection.Map[K, V]) extends AnyVal {
+ import scala.collection.compat._
+ def foreachKv[U](f: (K, V) => U): Unit = {
Review comment:
Or `foreachPair`.
----------------------------------------------------------------
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 merged pull request #9299: MINOR: Use `Map.forKeyValue` to avoid tuple allocation in Scala 2.13
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #9299:
URL: https://github.com/apache/kafka/pull/9299
----------------------------------------------------------------
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