Skip to content

Conversation

@nobodyiam
Copy link
Member

@nobodyiam nobodyiam commented Jan 17, 2026

What's the purpose of this PR

Replace synchronized multimap with concurrent hash map in NotificationControllerV2 for better performance

Which issue(s) this PR fixes:

Fixes #5450

Brief changelog

  • Replace synchronized multimap usage in NotificationControllerV2 with a concurrent, case-insensitive multimap implementation to reduce lock contention under high concurrency.
  • Introduce CaseInsensitiveMultimapWrapper (backed by ConcurrentMap + concurrent Set) to manage deferred results safely and efficiently; normalize keys in a locale-stable way.
  • Update NotificationControllerV2Test and add CaseInsensitiveMultimapWrapperTest to cover correctness and basic concurrency behavior.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Run mvn spotless:apply to format your code.
  • Update the CHANGES log.

Summary by CodeRabbit

  • Chores

    • Improved notification handling with a concurrent, case-insensitive backing to boost throughput and reduce lock contention.
  • New Features

    • Notification keys are now treated case-insensitively, improving matching consistency.
  • Tests

    • Added comprehensive unit tests covering put/get/remove, size, unmodifiable views, custom suppliers, and concurrency scenarios.
  • Documentation

    • Added a performance-focused changelog entry describing the notification handling improvements.

✏️ Tip: You can customize this high-level summary in your review settings.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 17, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

📝 Walkthrough

Walkthrough

Replaces a synchronized TreeMultimap in NotificationControllerV2 with a new CaseInsensitiveMultimapWrapper backed by concurrent map/set implementations; adds the wrapper class and comprehensive tests, updates NotificationControllerV2 and its test, and adds a changelog entry describing the concurrency/performance change.

Changes

Cohort / File(s) Summary
Core Implementation
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/wrapper/CaseInsensitiveMultimapWrapper.java
New generic case-insensitive multimap wrapper backed by Map<String, Set<V>> with pluggable map/set suppliers. Provides put, remove, get, containsKey, and size. Normalizes keys with Locale.ROOT.
Controller Update
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationControllerV2.java
Replaces Multimap<String, DeferredResultWrapper> synchronized TreeMultimap field with CaseInsensitiveMultimapWrapper<DeferredResultWrapper> using concurrent map and concurrent-hash-set supplier; imports adjusted.
Tests — New
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/wrapper/CaseInsensitiveMultimapWrapperTest.java
New unit tests covering put/get/remove semantics, case-insensitivity, size, unmodifiable get view, custom supplier behavior, and a concurrency race-condition scenario.
Tests — Updated
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/controller/NotificationControllerV2Test.java
Adjusted test field type and ReflectionTestUtils casts to CaseInsensitiveMultimapWrapper<DeferredResultWrapper>.
Changelog
CHANGES.md
Added Apollo 2.5.0 entry: "Perf: Replace synchronized multimap with concurrent hashmap in NotificationControllerV2 for better performance".

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

size:XL, lgtm

Suggested reviewers

  • hezhangjian

Poem

🐰
I swapped the locks for hopping haste,
Keys downcased, no time to waste,
Sets race safe, maps run free,
Deferred results now spring with glee,
A little hop for faster spree.

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the primary change: replacing synchronized multimap with concurrent data structure in NotificationControllerV2 for performance improvement.
Linked Issues check ✅ Passed The PR successfully addresses issue #5450 by replacing synchronized multimap with CaseInsensitiveMultimapWrapper backed by ConcurrentMap, reducing lock contention and improving performance under high concurrency.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the objective: the wrapper class, controller modifications, related test updates, and changelog entry are all necessary for implementing the concurrent multimap replacement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
`@apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/wrapper/CaseInsensitiveMultimapWrapperTest.java`:
- Around line 158-167: The test fails because
CaseInsensitiveMultimapWrapper.get(...) returns a Collections.unmodifiableSet,
so asserting set instanceof HashSet is false; change the testCustomSetSupplier
to unwrap the returned unmodifiable set via reflection (the
Collections$UnmodifiableSet has a private field "c" referencing the original
set) and assert that the underlying "c" is a HashSet, or alternatively access
the wrapper's internal map entry for the key and assert that its value is a
HashSet; reference the test method testCustomSetSupplier,
CaseInsensitiveMultimapWrapper.get, and the Collections$UnmodifiableSet "c"
field when making the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

状态检测接口出现偶发的线程阻塞问题

1 participant