You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cmccabe (via GitHub)" <gi...@apache.org> on 2024/04/30 21:09:23 UTC
[PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
cmccabe opened a new pull request, #15838:
URL: https://github.com/apache/kafka/pull/15838
Do not acquire the DynamicBrokerConfig lock in DynamicBrokerConfig.removeReconfigurable. It's not necessary, because the list that these functions are modifying is a thread-safe CopyOnWriteArrayList. In DynamicBrokerConfig.reloadUpdatedFilesWithoutConfigChange, I changed the code to use a simple Java forEach rather than a Scala conversion, in order to feel more confident that concurrent modifications to the List would not have any bad effects here. (forEach is always safe on CopyOnWriteArrayList.)
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #15838:
URL: https://github.com/apache/kafka/pull/15838#issuecomment-2137462449
The test should be fixed. I rebased on trunk. Thanks for the reviews @mumrah and @chia7712
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #15838:
URL: https://github.com/apache/kafka/pull/15838#issuecomment-2295370967
@cmccabe could you please rebase code to include the fix #16857?
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1619003590
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1579,6 +1579,36 @@ class KRaftClusterTest {
cluster.close()
}
}
+
+ @Test
+ def testReduceNumNetworkThreads(): Unit = {
Review Comment:
Could you please rename it to `testIncreaseNumNetworkThreads` by virtue of recent 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1586844632
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1541,6 +1541,36 @@ class KRaftClusterTest {
cluster.close()
}
}
+
+ @Test
+ def testReduceNumNetworkThreads(): Unit = {
Review Comment:
I wanted to ensure that no deadlocks are created in the future, by adding this test coverage. I don't think this PR is required to avoid deadlock currently, however.
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #15838:
URL: https://github.com/apache/kafka/pull/15838#issuecomment-2110742012
The new test seems to be failing: [`testReduceNumNetworkThreads() – kafka.server.KRaftClusterTest`](https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-15838/1/tests) can you check on 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1585911858
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1541,6 +1541,36 @@ class KRaftClusterTest {
cluster.close()
}
}
+
+ @Test
+ def testReduceNumNetworkThreads(): Unit = {
+ val cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setNumBrokerNodes(1).
+ setNumControllerNodes(1).build()).
+ setConfigProp(KafkaConfig.NumNetworkThreadsProp, "4").
+ build()
+ try {
+ cluster.format()
+ cluster.startup()
+ cluster.waitForReadyBrokers()
+ val admin = Admin.create(cluster.clientProperties())
+ try {
+ admin.incrementalAlterConfigs(
+ Collections.singletonMap(new ConfigResource(Type.BROKER, ""),
+ Collections.singletonList(new AlterConfigOp(
+ new ConfigEntry(KafkaConfig.NumNetworkThreadsProp, "2"), OpType.SET)))).all().get()
+ val newTopic = Collections.singletonList(new NewTopic("test-topic", 1, 1.toShort))
+ val createTopicResult = admin.createTopics(newTopic)
+ createTopicResult.all().get()
Review Comment:
It seems reducing the network threads causes the disconnection and it produces the retry of request forward. Hence, that can make this test unstable. Maybe we can increase the network threads instead?
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -303,17 +303,17 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
addBrokerReconfigurable(controller.socketServer)
}
- def addReconfigurable(reconfigurable: Reconfigurable): Unit = CoreUtils.inWriteLock(lock) {
+ def addReconfigurable(reconfigurable: Reconfigurable): Unit = {
verifyReconfigurableConfigs(reconfigurable.reconfigurableConfigs.asScala)
Review Comment:
It seems `verifyReconfigurableConfigs` does not need lock also, since it just check the input strings.
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1541,6 +1541,36 @@ class KRaftClusterTest {
cluster.close()
}
}
+
+ @Test
+ def testReduceNumNetworkThreads(): Unit = {
Review Comment:
Pardon me, is this test case related to deadlock?
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1586843624
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -303,17 +303,17 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
addBrokerReconfigurable(controller.socketServer)
}
- def addReconfigurable(reconfigurable: Reconfigurable): Unit = CoreUtils.inWriteLock(lock) {
+ def addReconfigurable(reconfigurable: Reconfigurable): Unit = {
verifyReconfigurableConfigs(reconfigurable.reconfigurableConfigs.asScala)
Review Comment:
Good catch.
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1608469438
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -303,17 +303,17 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
addBrokerReconfigurable(controller.socketServer)
}
- def addReconfigurable(reconfigurable: Reconfigurable): Unit = CoreUtils.inWriteLock(lock) {
+ def addReconfigurable(reconfigurable: Reconfigurable): Unit = {
verifyReconfigurableConfigs(reconfigurable.reconfigurableConfigs.asScala)
Review Comment:
@cmccabe Can we remove the lock from `verifyReconfigurableConfigs` in this PR? Or we can address that in next one.
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #15838:
URL: https://github.com/apache/kafka/pull/15838#issuecomment-2110829824
> The new test seems to be failing: [testReduceNumNetworkThreads() – kafka.server.KRaftClusterTest](https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-15838/1/tests) can you check on it?
see https://github.com/apache/kafka/pull/15838#discussion_r1585911858
--
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
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #15838:
URL: https://github.com/apache/kafka/pull/15838#discussion_r1711855107
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -303,17 +303,17 @@ class DynamicBrokerConfig(private val kafkaConfig: KafkaConfig) extends Logging
addBrokerReconfigurable(controller.socketServer)
}
- def addReconfigurable(reconfigurable: Reconfigurable): Unit = CoreUtils.inWriteLock(lock) {
+ def addReconfigurable(reconfigurable: Reconfigurable): Unit = {
verifyReconfigurableConfigs(reconfigurable.reconfigurableConfigs.asScala)
Review Comment:
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16649: Remove lock from DynamicBrokerConfig.removeReconfigurable [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #15838:
URL: https://github.com/apache/kafka/pull/15838#issuecomment-2089167707
> Thanks for the patch, @cmccabe. I looked for other usages of this lock and see we're obtaining the write lock in DynamicBrokerConfig#updateBrokerConfig which gets called from DynamicConfigPublisher. Will we still need this locking when we have dropped ZK?
I think we can simplify this code a lot once ZK is gone, yes. Having a single updater will help a lot. I suspect we can get rid of the lock at that point, although I'll have to look more later.
> Can you add a comment next to the lock indicating what it is guarding?
I added a clarification that the lock protects against concurrent reconfiguration operations, but not concurrent additions or removals of reconfigurables. As you mentioned, this is mostly only relevant to ZK mode since there we can have racing threads.
--
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