Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison merged PR #15786: URL: https://github.com/apache/kafka/pull/15786 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2077391342 Quite a few failures in the last CI run but it seems to be flaky tests as none of the failures happened on all platforms. Merging to trunk -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2076660780 Just rebased, let's run another build -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
chia7712 commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2076135599 > It looks like the previous CI build had an issue with the Java 8/Scala 2.12 pipeline. I rekicked a build. The root cause is that some services are not closed. I file a PR (#15801) to fix that. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
chia7712 commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2075606243 > It looks like the previous CI build had an issue with the Java 8/Scala 2.12 pipeline. I rekicked a build. oh, I rekicked it too :_ -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2075580384 It looks like the previous CI build had an issue with the Java 8/Scala 2.12 pipeline. I rekicked a build. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
chia7712 commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577684664
##
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala:
##
@@ -347,7 +347,7 @@ abstract class KafkaServerTestHarness extends
QuorumTestHarness {
if (isKRaftTest()) {
val result = new util.HashMap[Uuid, String]()
controllerServer.controller.findAllTopicIds(ANONYMOUS_CONTEXT).get().entrySet().forEach
{
Review Comment:
I file https://issues.apache.org/jira/browse/KAFKA-16610 to trace 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577677934
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -2192,8 +2192,8 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
@ValueSource(strings = Array("zk", "kraft"))
def testLongTopicNames(quorum: String): Unit = {
val client = Admin.create(createConfig)
-val longTopicName = String.join("", Collections.nCopies(249, "x"));
-val invalidTopicName = String.join("", Collections.nCopies(250, "x"));
+val longTopicName = String.join("", Collections.nCopies(249, "x"))
Review Comment:
That's a nice trick, thanks! Done
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577660497
##
core/src/main/scala/kafka/zookeeper/ZooKeeperClient.scala:
##
@@ -52,8 +51,8 @@ object ZooKeeperClient {
* @param sessionTimeoutMs session timeout in milliseconds
* @param connectionTimeoutMs connection timeout in milliseconds
* @param maxInFlightRequests maximum number of unacknowledged requests the
client will send before blocking.
+ * @param clientConfig ZooKeeper client configuration, for TLS configs if
desired
Review Comment:
To be honest, I considered not touching that file at all since we expect to
delete it soon. I wouldn't spend too much time cleaning it up, so I'll ignore
these other issues.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577656938
##
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala:
##
@@ -347,7 +347,7 @@ abstract class KafkaServerTestHarness extends
QuorumTestHarness {
if (isKRaftTest()) {
val result = new util.HashMap[Uuid, String]()
controllerServer.controller.findAllTopicIds(ANONYMOUS_CONTEXT).get().entrySet().forEach
{
Review Comment:
Yeah I spotted potential refactorings around `forEach()`. I think this PR is
already large enough. If we decide to change these, let's do it in another PR.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577653920
##
core/src/main/scala/kafka/metrics/LinuxIoMetricsCollector.scala:
##
@@ -29,9 +29,9 @@ import scala.jdk.CollectionConverters._
*/
class LinuxIoMetricsCollector(procRoot: String, val time: Time, val logger:
Logger) {
import LinuxIoMetricsCollector._
- var lastUpdateMs: Long = -1L
- var cachedReadBytes:Long = 0L
- var cachedWriteBytes:Long = 0L
+ private var lastUpdateMs: Long = -1L
Review Comment:
Right, now that these are private, we can drop the types. Done
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1577491649
##
core/src/main/scala/kafka/server/SharedServer.scala:
##
@@ -107,16 +107,16 @@ class SharedServer(
@volatile var brokerMetrics: BrokerServerMetrics = _
@volatile var controllerServerMetrics: ControllerMetadataMetrics = _
@volatile var loader: MetadataLoader = _
- val snapshotsDisabledReason = new AtomicReference[String](null)
+ private val snapshotsDisabledReason = new AtomicReference[String](null)
@volatile var snapshotEmitter: SnapshotEmitter = _
- @volatile var snapshotGenerator: SnapshotGenerator = _
- @volatile var metadataLoaderMetrics: MetadataLoaderMetrics = _
+ @volatile private var snapshotGenerator: SnapshotGenerator = _
+ @volatile private var metadataLoaderMetrics: MetadataLoaderMetrics = _
def clusterId: String = metaPropsEnsemble.clusterId().get()
- def nodeId: Int = metaPropsEnsemble.nodeId().getAsInt()
+ def nodeId: Int = metaPropsEnsemble.nodeId().getAsInt
- def isUsed(): Boolean = synchronized {
+ private def isUsed(): Boolean = synchronized {
Review Comment:
Yeah Scala seems to favor not using braces for 0 arguments getters. I've not
made that change as we have a lot of instances of these and that would make a
huge diff.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
chia7712 commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576457731
##
core/src/main/scala/kafka/metrics/LinuxIoMetricsCollector.scala:
##
@@ -29,9 +29,9 @@ import scala.jdk.CollectionConverters._
*/
class LinuxIoMetricsCollector(procRoot: String, val time: Time, val logger:
Logger) {
import LinuxIoMetricsCollector._
- var lastUpdateMs: Long = -1L
- var cachedReadBytes:Long = 0L
- var cachedWriteBytes:Long = 0L
+ private var lastUpdateMs: Long = -1L
Review Comment:
It seems the type declaration is unnecessary since its literal ends with `L`
##
core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala:
##
@@ -347,7 +347,7 @@ abstract class KafkaServerTestHarness extends
QuorumTestHarness {
if (isKRaftTest()) {
val result = new util.HashMap[Uuid, String]()
controllerServer.controller.findAllTopicIds(ANONYMOUS_CONTEXT).get().entrySet().forEach
{
Review Comment:
how about
`controllerServer.controller.findAllTopicIds(ANONYMOUS_CONTEXT).get().forEach((k,
v) => result.put(v, k))`
##
core/src/main/scala/kafka/server/SharedServer.scala:
##
@@ -107,16 +107,16 @@ class SharedServer(
@volatile var brokerMetrics: BrokerServerMetrics = _
@volatile var controllerServerMetrics: ControllerMetadataMetrics = _
@volatile var loader: MetadataLoader = _
- val snapshotsDisabledReason = new AtomicReference[String](null)
+ private val snapshotsDisabledReason = new AtomicReference[String](null)
@volatile var snapshotEmitter: SnapshotEmitter = _
- @volatile var snapshotGenerator: SnapshotGenerator = _
- @volatile var metadataLoaderMetrics: MetadataLoaderMetrics = _
+ @volatile private var snapshotGenerator: SnapshotGenerator = _
+ @volatile private var metadataLoaderMetrics: MetadataLoaderMetrics = _
def clusterId: String = metaPropsEnsemble.clusterId().get()
- def nodeId: Int = metaPropsEnsemble.nodeId().getAsInt()
+ def nodeId: Int = metaPropsEnsemble.nodeId().getAsInt
- def isUsed(): Boolean = synchronized {
+ private def isUsed(): Boolean = synchronized {
Review Comment:
How about `isUsed`?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576422765
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -959,7 +960,7 @@ class ZkAdminManager(val config: KafkaConfig,
} else if (requestStatus.mechanism == Some(ScramMechanism.UNKNOWN)) {
(requestStatus.user, unknownScramMechanismMsg)
Review Comment:
It looks weird to me but it seems `contains()` is Scala idiomatic, so I made
the 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786: URL: https://github.com/apache/kafka/pull/15786#discussion_r1576420731 ## core/src/main/scala/kafka/server/ReplicaManager.scala: ## @@ -144,7 +144,7 @@ case class LogReadResult(info: FetchDataInfo, def withEmptyFetchInfo: LogReadResult = Review Comment: You're right, this is unused -> removed -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576375031
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -2192,8 +2192,8 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
@ValueSource(strings = Array("zk", "kraft"))
def testLongTopicNames(quorum: String): Unit = {
val client = Admin.create(createConfig)
-val longTopicName = String.join("", Collections.nCopies(249, "x"));
-val invalidTopicName = String.join("", Collections.nCopies(250, "x"));
+val longTopicName = String.join("", Collections.nCopies(249, "x"))
Review Comment:
Small scala suggestion here (which feel free to ignore) we can use
`List.fill(249)("x").mkString("")` instead of Java `String.join` and
`Collections.nCopies`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576375031
##
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##
@@ -2192,8 +2192,8 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
@ValueSource(strings = Array("zk", "kraft"))
def testLongTopicNames(quorum: String): Unit = {
val client = Admin.create(createConfig)
-val longTopicName = String.join("", Collections.nCopies(249, "x"));
-val invalidTopicName = String.join("", Collections.nCopies(250, "x"));
+val longTopicName = String.join("", Collections.nCopies(249, "x"))
Review Comment:
Small scala suggestion here (which you feel free to ignore) we can use
`List.fill(249)("x").mkString("")` instead of Java `String.join` and
`Collections.nCopies`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576366593
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
make sense don't worry about it. We are moving more and more from scala to
java anyway so these will get resolved over time.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576363327
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
That's just my guesstimate, but I expect it to be large. I kind of focused
on the low hanging fruits 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576360092
##
core/src/main/scala/kafka/zookeeper/ZooKeeperClient.scala:
##
@@ -52,8 +51,8 @@ object ZooKeeperClient {
* @param sessionTimeoutMs session timeout in milliseconds
* @param connectionTimeoutMs connection timeout in milliseconds
* @param maxInFlightRequests maximum number of unacknowledged requests the
client will send before blocking.
+ * @param clientConfig ZooKeeper client configuration, for TLS configs if
desired
Review Comment:
Nice, I believe we might need to update the have java doc in
`registerStateChangeHandler` and `unregisterStateChangeHandler` in same file 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786: URL: https://github.com/apache/kafka/pull/15786#discussion_r1576358398 ## core/src/main/scala/kafka/server/ReplicaManager.scala: ## @@ -300,7 +300,7 @@ class ReplicaManager(val config: KafkaConfig, protected val allPartitions = new Pool[TopicPartition, HostedPartition]( Review Comment: Ideally we should declare types for all public and protected fields but this is a huge change. Also while it's useful in some cases, in many I find it's not adding much value. In this specific example I even find it annoying as you get: ``` protected val allPartitions: Pool[TopicPartition, HostedPartition] = new Pool[TopicPartition, HostedPartition]( ``` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576351984
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
Yeah in Scala braces are not required around multi-line blocks. I've not
made this change because braces are required in Java and we have the braces in
Scala all over the code base. Changing this is probably a >500 line diff.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on PR #15786: URL: https://github.com/apache/kafka/pull/15786#issuecomment-2072445566 We also have couple of out-of-date parameters in javaDocs, we can either fix here or have another followup pr - https://github.com/apache/kafka/blob/1b301b30207ed8fca9f0aea5cf940b0353a1abca/core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala#L150 - https://github.com/apache/kafka/blob/1b301b30207ed8fca9f0aea5cf940b0353a1abca/core/src/main/scala/kafka/server/metadata/KRaftMetadataCache.scala#L270 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576354443
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
ough >500 is too much. Well they will get cleanup as we move more scala to
java :D
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786: URL: https://github.com/apache/kafka/pull/15786#discussion_r1576343801 ## core/src/main/scala/kafka/server/ReplicaManager.scala: ## @@ -300,7 +300,7 @@ class ReplicaManager(val config: KafkaConfig, protected val allPartitions = new Pool[TopicPartition, HostedPartition]( Review Comment: Should we declare type for protected (and the other public variables above) -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786: URL: https://github.com/apache/kafka/pull/15786#discussion_r1576338028 ## core/src/main/scala/kafka/server/ReplicaManager.scala: ## @@ -144,7 +144,7 @@ case class LogReadResult(info: FetchDataInfo, def withEmptyFetchInfo: LogReadResult = Review Comment: This is not used as far as I can see, do we still need 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576333034
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
We have this in few places in this class 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576332250
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -871,7 +872,7 @@ class ZkAdminManager(val config: KafkaConfig,
users.get.filterNot(usersToSkip.contains).foreach { user =>
try {
val userConfigs = adminZkClient.fetchEntityConfig(ConfigType.USER,
Sanitizer.sanitize(user))
-addToResultsIfHasScramCredential(user, userConfigs, true)
+addToResultsIfHasScramCredential(user, userConfigs, explicitUser =
true)
} catch {
case e: Exception => {
Review Comment:
the braces here is redundant not sure if we want to clean this up as well or
not
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
OmniaGM commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576329465
##
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##
@@ -959,7 +960,7 @@ class ZkAdminManager(val config: KafkaConfig,
} else if (requestStatus.mechanism == Some(ScramMechanism.UNKNOWN)) {
(requestStatus.user, unknownScramMechanismMsg)
Review Comment:
the condition above should use `.contains` instead of `== Some`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
mimaison commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576233041
##
core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala:
##
@@ -209,7 +209,7 @@ class GroupCoordinatorConcurrencyTest extends
AbstractCoordinatorConcurrencyTest
class JoinGroupOperation extends GroupOperation[JoinGroupCallbackParams,
JoinGroupCallback] {
override def responseCallback(responsePromise:
Promise[JoinGroupCallbackParams]): JoinGroupCallback = {
- val callback: JoinGroupCallback = responsePromise.success(_)
+ val callback: JoinGroupCallback = responsePromise.success
Review Comment:
Yup, fixed!
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Various cleanups in core [kafka]
chia7712 commented on code in PR #15786:
URL: https://github.com/apache/kafka/pull/15786#discussion_r1576193128
##
core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala:
##
@@ -209,7 +209,7 @@ class GroupCoordinatorConcurrencyTest extends
AbstractCoordinatorConcurrencyTest
class JoinGroupOperation extends GroupOperation[JoinGroupCallbackParams,
JoinGroupCallback] {
override def responseCallback(responsePromise:
Promise[JoinGroupCallbackParams]): JoinGroupCallback = {
- val callback: JoinGroupCallback = responsePromise.success(_)
+ val callback: JoinGroupCallback = responsePromise.success
Review Comment:
This causes build error when using scala 2.12
```
[Error]
/home/chia7712/project/kafka/core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala:212:57:
method with dependent type (value:
kafka.coordinator.group.GroupCoordinatorConcurrencyTest.JoinGroupCallbackParams)responsePromise.type
cannot be converted to function value
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
