Provide LocalMapStats data for invocations from Lite members [HZ-2072] #24871
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**Context:** Metrics for IMaps are stored in `LocalMapStats` objects, which are provided by `LocalMapStatsProvider`. Certain MapOperation metrics (get, set, put, & remove) are logged when `MapProxySupport#invokeOperation` is called, which uses `MapOperationStatsUpdater.incrementOperationStats`. This happens for operations not executed on the local member as well, which means that a Lite member invoking `IMap#put` will cause a `PutOperation` being executed on another (non-Lite) member, but it will increment stats on its own member. **Problem:** Due to Lite members not owning any data themselves, they do not report any metrics for MapOperations. This means that metrics on these operations are lost with the current metric collection mechanics. **Solution:** Tweaking `LocalMapStatsProvider#addStatsOfNoDataIncludedMaps` to include a `isLiteMember` check which, if passed, will include metrics from the provider's `statsMap` field (which is already correctly updated). This is only done if an empty `LocalMapStats` object would've been passed otherwise, and the `statsMap` entry contains non-zero statistics. I have also added a very simple regression test which ensures Lite members produce metrics after `IMap#put` is called. **Note:** It may be worth reworking our map statistic capturing mechanic, shifting the responsibility to the executing member rather than the invoker. I chose my approach with the mindset of changing the least amount of existing functionality/behaviour, while also stopping these metrics being lost entirely as they currently are.
JamesHazelcast
added
Type: Defect
Team: Core
Source: Internal
|
Leaving as a draft request for now as I'm looking for feedback on this approach and whether there's any considerations I've missed. |
ahmetmircik
reviewed
Jun 22, 2023
hazelcast/src/main/java/com/hazelcast/map/impl/LocalMapStatsProvider.java
Outdated
Show resolved
Hide resolved
Based on Ahmet's feedback from my PR - testing suggests that after a Lite member invokes a `PutOperation`, it does get a `MapContainer` object, meaning that cleanup of the `LocalMapStatsImpl` is already handled in the existing `MapServiceContextImpl#destroyMap` function. I made an addition to my regression test which tests for this and confirms this destruction.
ahmetmircik
approved these changes
Jun 23, 2023
hazelcast/src/main/java/com/hazelcast/map/impl/LocalMapStatsProvider.java
Outdated
Show resolved
Hide resolved
The `MapProxySupport#getLocalMapStats` method is creative, so if there was no `LocalMapStatsImpl` created prior, it would be created - also under the hood it calls back to the `LocalMapStatsProvider` to check its `statsMap` field anyway, so we can cut out the middle man and check the `statsMap` field directly. If no existing stats exist in the `statsMap` map, then an empty `LocalMapStatsImpl` is used instead - this maintains any prior functionality related to "empty stats". My testing also discovered that our current `MapServiceContext#destroyMap` method does not account for Lite members that never access a Map (which means they never get a `MapContainer`), but do get a `LocalMapStatsImpl` created when their map proxy is created. As they have no `MapContainer`, the `MapServiceContext#destroyMap` method does not reach the calls where the stats are cleaned up - this commit resolves that by calling the removal function even if there is no `MapContainer` present.
vbekiaris
approved these changes
Jun 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: Metrics for IMaps are stored in
LocalMapStatsobjects, which are provided byLocalMapStatsProvider. Certain MapOperation metrics (get, set, put, & remove) are logged whenMapProxySupport#invokeOperationis called, which usesMapOperationStatsUpdater.incrementOperationStats. This happens for operations not executed on the local member as well, which means that a Lite member invokingIMap#putwill cause aPutOperationbeing executed on another (non-Lite) member, but it will increment stats on its own member.Problem: Due to Lite members not owning any data themselves, they do not report any metrics for MapOperations. This means that metrics on these operations are lost with the current metric collection mechanics.
Original Solution: Tweaking
LocalMapStatsProvider#addStatsOfNoDataIncludedMapsto include aisLiteMembercheck which, if passed, will include metrics from the provider'sstatsMapfield (which is already correctly updated). This is only done if an emptyLocalMapStatsobject would've been passed otherwise, and thestatsMapentry contains non-zero statistics.Revised Solution: Tweaking
LocalMapStatsProvider#addStatsOfNoDataIncludedMapsto provide the localstatsMapvalue ofLocalMapStatsImplwhen there is no data currently included - thisLocalMapStatsImplcontains the invocation counts from Lite members (which is already correctly updated). This is only done if an emptyLocalMapStatsobject would've been passed otherwise, and thestatsMapvalue is not null.While testing I also identified a scenario where a Lite member can have its own
LocalMapStatsImplfor a map which it does not currently hold aMapContainerfor - this means the stats object is not cleaned up. I made changes toMapServiceContextImpl#destroyMapto account for this and clean up these objects on Lite members.I have also added a very simple regression test which ensures Lite members produce metrics after
IMap#putis called.Note: It may be worth reworking our map statistic capturing mechanic, shifting the responsibility to the executing member rather than the invoker. I chose my approach with the mindset of changing the least amount of existing functionality/behaviour, while also stopping these metrics being lost entirely as they currently are.
Closes https://hazelcast.atlassian.net/browse/HZ-2072