Skip to content

MINOR: Refactor brokerContactTimesMs and brokerRegistrationStates to use Long and Integer #19888

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.

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

Conversation

apalan60
Copy link
Contributor

@apalan60 apalan60 commented Jun 3, 2025

This PR simplifies two ConcurrentHashMap fields by removing their Atomic
wrappers:

  • Change brokerContactTimesMs from ConcurrentHashMap<Integer, AtomicLong> to ConcurrentHashMap<Integer, Long>.

  • Change brokerRegistrationStates from ConcurrentHashMap<Integer, AtomicInteger> to ConcurrentHashMap<Integer, Integer>.

This removes mutable holders without affecting thread safety (see
discussion in #19828).

Reviewers: Chia-Ping Tsai chia7712@gmail.com, TengYao Chi frankvicky@apache.org, Kevin Wu kevin.wu2412@gmail.com, Ken Huang s7133700@gmail.com

@github-actions github-actions bot added triage PRs from the community kraft small Small PRs labels Jun 3, 2025
@apalan60 apalan60 changed the title [WIP] MINOR: Refactor brokerContactTimesMs from AtomicLong to Long MINOR: Refactor brokerContactTimesMs from AtomicLong to Long Jun 3, 2025
@apalan60 apalan60 marked this pull request as ready for review June 3, 2025 18:34
@Yunyung
Copy link
Collaborator

Yunyung commented Jun 3, 2025

private final Map<Integer, AtomicInteger> brokerRegistrationStates = new ConcurrentHashMap<>();

This is the same situation.

@chia7712
Copy link
Member

chia7712 commented Jun 4, 2025

@kevin-wu24 could you please take a look?

@apalan60 apalan60 changed the title MINOR: Refactor brokerContactTimesMs from AtomicLong to Long MINOR: Refactor brokerContactTimesMs and brokerRegistrationStates to use Long and Integer Jun 4, 2025
@apalan60
Copy link
Contributor Author

apalan60 commented Jun 5, 2025

This is the same situation.

@Yunyung
Thanks for your review. I’ve made the same change to brokerRegistrationStates as well.

Copy link
Contributor

@kevin-wu24 kevin-wu24 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@github-actions github-actions bot removed the triage PRs from the community label Jun 5, 2025
@frankvicky frankvicky merged commit aaed164 into apache:trunk Jun 6, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants