Skip to content

MINOR: Replace Map conversions with MetricsUtils.getTags for metric tags#21948

Merged
chia7712 merged 19 commits intoapache:trunkfrom
TaiJuWu:KAFKA-18390-1
Apr 7, 2026
Merged

MINOR: Replace Map conversions with MetricsUtils.getTags for metric tags#21948
chia7712 merged 19 commits intoapache:trunkfrom
TaiJuWu:KAFKA-18390-1

Conversation

@TaiJuWu
Copy link
Copy Markdown
Collaborator

@TaiJuWu TaiJuWu commented Apr 2, 2026 •
edited by github-actions Bot
Loading

Replace HashMap with LinkedHashMap for metrics tag maps so that JMX
MBean names always reflect a deterministic, insertion-order tag
sequence. Also migrate Scala core module tag construction to use the
existing MetricsUtils.getTags(...) helper for consistency.

Reviewers: Chia-Ping Tsai chia7712@gmail.com, Ken Huang
s7133700@gmail.com, Chia-Yi Chiu cychiu8@gmail.com

@github-actions github-actions Bot added triage PRs from the community streams core Kafka Broker consumer kraft storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature clients small Small PRs labels Apr 2, 2026
@github-actions github-actions Bot removed the small Small PRs label Apr 2, 2026
@TaiJuWu TaiJuWu changed the title (WIP) KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder Apr 3, 2026
@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Apr 3, 2026

@TaiJuWu please fix conflicts

chia7712
chia7712 reviewed Apr 4, 2026
View reviewed changes
Comment thread metadata/src/main/java/org/apache/kafka/image/TopicDelta.java Outdated
private final Map<Integer, PartitionRegistration> partitionChanges = new HashMap<>();
private final Map<Integer, Integer> partitionToUncleanLeaderElectionCount = new HashMap<>();
private final Map<Integer, Integer> partitionToElrElectionCount = new HashMap<>();
private final Map<Integer, Integer> partitionToUncleanLeaderElectionCount = new LinkedHashMap<>();
Copy link
Copy Markdown
Member

@chia7712 chia7712 Apr 4, 2026

Choose a reason for hiding this comment

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

why make this change?

Copy link
Copy Markdown
Collaborator Author

@TaiJuWu TaiJuWu Apr 4, 2026

Choose a reason for hiding this comment

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

Mistake :(

Comment thread metadata/src/main/java/org/apache/kafka/image/TopicDelta.java Outdated
private final Map<Integer, Integer> partitionToUncleanLeaderElectionCount = new HashMap<>();
private final Map<Integer, Integer> partitionToElrElectionCount = new HashMap<>();
private final Map<Integer, Integer> partitionToUncleanLeaderElectionCount = new LinkedHashMap<>();
private final Map<Integer, Integer> partitionToElrElectionCount = new LinkedHashMap<>();
Copy link
Copy Markdown
Member

@chia7712 chia7712 Apr 4, 2026

Choose a reason for hiding this comment

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

ditto

TaiJuWu
TaiJuWu commented Apr 4, 2026
View reviewed changes
Comment thread server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java Outdated
}

Map<String, Pattern> patternsMap = new HashMap<>();
Map<String, Pattern> patternsMap = new LinkedHashMap<>();
Copy link
Copy Markdown
Collaborator Author

@TaiJuWu TaiJuWu Apr 4, 2026

Choose a reason for hiding this comment

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

test

Copy link
Copy Markdown
Contributor

@cychiu8 cychiu8 Apr 4, 2026

Choose a reason for hiding this comment

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

test 2

cychiu8
cychiu8 reviewed Apr 4, 2026
View reviewed changes
Comment thread server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsInstanceMetadata.java Outdated
Objects.requireNonNull(requestContext);

attributesMap = new HashMap<>();
attributesMap = new LinkedHashMap<>();
Copy link
Copy Markdown
Contributor

@cychiu8 cychiu8 Apr 4, 2026

Choose a reason for hiding this comment

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

when will we use new LinkedHashMap directly and when will we use MetricsUtils.getTags? do we need to align the situations?

Copy link
Copy Markdown
Collaborator Author

@TaiJuWu TaiJuWu Apr 4, 2026

Choose a reason for hiding this comment

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

There is no strict limitations we should select unless scala code. I just select minimal change during this patch.

Copy link
Copy Markdown
Member

@chia7712 chia7712 Apr 5, 2026

Choose a reason for hiding this comment

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

attributesMap is not used as tags, right? If so, why do we make this change?

Copy link
Copy Markdown
Collaborator Author

@TaiJuWu TaiJuWu Apr 6, 2026

Choose a reason for hiding this comment

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

Look like I misunderstood something, will review and test this PR.

cychiu8
cychiu8 reviewed Apr 4, 2026
View reviewed changes
Comment thread clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchMetricsAggregator.java Outdated
private final Set<TopicPartition> unrecordedPartitions;
private final FetchMetrics fetchFetchMetrics = new FetchMetrics();
private final Map<String, FetchMetrics> perTopicFetchMetrics = new HashMap<>();
private final Map<String, FetchMetrics> perTopicFetchMetrics = new LinkedHashMap<>();
Copy link
Copy Markdown
Contributor

@cychiu8 cychiu8 Apr 4, 2026

Choose a reason for hiding this comment

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

test test

@github-actions github-actions Bot removed the triage PRs from the community label Apr 5, 2026
@github-actions github-actions Bot added the small Small PRs label Apr 6, 2026
@TaiJuWu
Copy link
Copy Markdown
Collaborator Author

TaiJuWu commented Apr 6, 2026

Test change by CI.

@TaiJuWu TaiJuWu changed the title KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder ( WIP)KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder Apr 6, 2026
@TaiJuWu TaiJuWu changed the title ( WIP)KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder Apr 6, 2026
@chia7712 chia7712 changed the title KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder KAFKA-18390 Replace Map conversions with MetricsUtils.getTags for metric tags Apr 7, 2026
@chia7712 chia7712 changed the title KAFKA-18390 Replace Map conversions with MetricsUtils.getTags for metric tags MINOR: Replace Map conversions with MetricsUtils.getTags for metric tags Apr 7, 2026
@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Apr 7, 2026

I've updated the PR title since the original proposal was to change the parameter type from Map to LinkedHashMap. This PR doesn't fully address that scope yet.

@chia7712 chia7712 merged commit 20d975a into apache:trunk Apr 7, 2026
25 checks passed
@TaiJuWu TaiJuWu deleted the KAFKA-18390-1 branch April 7, 2026 21:34
nileshkumar3 pushed a commit to nileshkumar3/kafka that referenced this pull request Apr 15, 2026
@TaiJuWu @nileshkumar3
MINOR: Replace Map conversions with MetricsUtils.getTags for metric t…
0dea606 
…ags (apache#21948)

Replace HashMap with LinkedHashMap for metrics tag maps so that JMX
MBean names always reflect a deterministic,  insertion-order tag
sequence. Also migrate Scala core module tag  construction to use the
existing MetricsUtils.getTags(...) helper for  consistency.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Ken Huang
 <s7133700@gmail.com>, Chia-Yi Chiu <cychiu8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

+2 more reviewers

@cychiu8 cychiu8 cychiu8 approved these changes

@m1a2st m1a2st m1a2st approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

clients consumer core Kafka Broker kraft small Small PRs storage Pull requests that target the storage module streams tiered-storage Related to the Tiered Storage feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TaiJuWu @chia7712 @cychiu8 @m1a2st