Skip to content

MINOR: Remove threadNamePrefix parameter from ReplicaManager and ReplicaFetcherManager #20069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 1, 2025
Merged

MINOR: Remove threadNamePrefix parameter from ReplicaManager and ReplicaFetcherManager #20069

merged 3 commits into from
Jul 1, 2025

Conversation

dalaoqi
Copy link
Contributor

@dalaoqi dalaoqi commented Jun 30, 2025 •
edited by github-actions bot
Loading

  • remove threadNamePrefix from ReplicaManager constructor
  • update BrokerServer to use updated constructor
  • remove threadNamePrefix from ReplicaFetcherManager

Reviewers: PoAn Yang payang@apache.org, TengYao Chi
frankvicky@apache.org
@github-actions github-actions bot added triage PRs from the community core Kafka Broker small Small PRs labels Jun 30, 2025
@dalaoqi dalaoqi changed the title Remove threadNamePrefix parameter from ReplicaManager and ReplicaFetc… Jun 30, 2025
@dalaoqi dalaoqi marked this pull request as ready for review June 30, 2025 09:41
@dalaoqi dalaoqi changed the title MINOR: Remove threadNamePrefix parameter from ReplicaManager and ReplicaFetc… Jun 30, 2025
@chia7712 chia7712 self-requested a review June 30, 2025 09:52
FrankYang0529
FrankYang0529 reviewed Jun 30, 2025
View reviewed changes
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dalaoqi Please run ./gradlew checkstyleMain checkstyleTest spotlessCheck to fix lint issue. Thanks.
frankvicky
frankvicky reviewed Jun 30, 2025
View reviewed changes
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dalaoqi: Thanks for the patch!
core/src/main/scala/kafka/server/ReplicaFetcherManager.scala
@@ -28,7 +28,6 @@ class ReplicaFetcherManager(brokerConfig: KafkaConfig,
protected val replicaManager: ReplicaManager,
metrics: Metrics,
time: Time,
threadNamePrefix: Option[String] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You delete the threadNamePrefix from the constructor, but you don't update ReplicaManagerBuilder correspondingly. Hence, the build fails.

kafka/core/src/main/java/kafka/server/builders/ReplicaManagerBuilder.java

Lines 112 to 134 in 64aebb5

return new ReplicaManager(config,
metrics,
time,
scheduler,
logManager,
Option.empty(),
quotaManagers,
metadataCache,
logDirFailureChannel,
alterPartitionManager,
brokerTopicStats,
Option.empty(),
Option.empty(),
Option.empty(),
Option.empty(),
Option.empty(),
Option.empty(),
Option.empty(),
() -> -1L,
Option.empty(),
DirectoryEventHandler.NOOP,
new DelayedActionQueue());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractCoordinatorConcurrencyTest

kafka/core/src/test/scala/unit/kafka/coordinator/AbstractCoordinatorConcurrencyTest.scala

Line 194 in 64aebb5

threadNamePrefix = Option(this.getClass.getName)) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReplicaManagerConcurrencyTest

kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerConcurrencyTest.scala

Line 203 in 64aebb5

threadNamePrefix: Option[String],

frankvicky
frankvicky reviewed Jun 30, 2025
View reviewed changes
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dalaoqi: The clean-up patch is easy to miss things. You can build the patch on your local machine to see if it breaks anything.
./gradlew clean build -x test
core/src/main/scala/kafka/server/ReplicaFetcherManager.scala
@@ -28,7 +28,6 @@ class ReplicaFetcherManager(brokerConfig: KafkaConfig,
protected val replicaManager: ReplicaManager,
metrics: Metrics,
time: Time,
threadNamePrefix: Option[String] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractCoordinatorConcurrencyTest

kafka/core/src/test/scala/unit/kafka/coordinator/AbstractCoordinatorConcurrencyTest.scala

Line 194 in 64aebb5

threadNamePrefix = Option(this.getClass.getName)) {

core/src/main/scala/kafka/server/ReplicaFetcherManager.scala
@@ -28,7 +28,6 @@ class ReplicaFetcherManager(brokerConfig: KafkaConfig,
protected val replicaManager: ReplicaManager,
metrics: Metrics,
time: Time,
threadNamePrefix: Option[String] = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReplicaManagerConcurrencyTest

kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerConcurrencyTest.scala

Line 203 in 64aebb5

threadNamePrefix: Option[String],

@github-actions github-actions bot removed the triage PRs from the community label Jul 1, 2025
frankvicky
frankvicky approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dalaoqi: LGTM
@frankvicky frankvicky merged commit ad934d3 into apache:trunk Jul 1, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker small Small PRs
4 participants
@dalaoqi @FrankYang0529 @frankvicky @chia7712