Skip to content
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

Use new instance of cache map instead of cleaning #5473

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

niksv
Copy link
Collaborator

@niksv niksv commented Nov 14, 2023

Creation of a new map instance was used instead of map.clear() to clear a cache. We need it because map.clear() doesn't shrink already allocated map capacity, size of which can be significant.

INFO for QAs:
Following topologies was updated:

  • otsdb
  • flow monitoring
  • stats

How to check that PR works correctly:

  1. Create a flow.
  2. Deactivate and activate back each topology. Here you can find how to do it: Missed cache for HA flow meter after stats topology restart #5455 (step 2).
  3. Check
  • stats topology - there are fresh otsdb/victoria metrics for the flow like kilda.flow.packets
  • flow monitoring - there are fresh otsdb/victoria metrics kilda.flow.rtt with tag origin=flow-monitoring (can be missed if server42 is active for the flow)
  • otsdb topology - there are any fresh metric in otsdb/victoria

Creation of a new map instance was used instead of map.clear() to clear a cache.
We need it because map.clear() doesn't shrink already allocated map capacity,
size of which can be significant.
public void removeOutdated(long ttlInMillis) {
long now = System.currentTimeMillis();
map.entrySet().removeIf(entry -> now - entry.getValue().getTime() > ttlInMillis);
if (map.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to resize the map anyway, because if there is one element left, we keep a map with one element and some bigger capacity.
There is a HashMap constructor taking a map that creates a new map with capacity just enough to accommodate the existing elements. As an option, it could be done either if the map is empty or the size is small enough so that the copy is not too expensive.

Copy link
Collaborator Author

@niksv niksv Nov 15, 2023

Choose a reason for hiding this comment

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

Good point, but it is very rare case because to face with the case we must

  1. Had a significant capacity before.
  2. Expire most of datapoints, but not all of them
  3. Be in this state for a long time. (in other case the last datapoint will expire too).

I can't imagine how we can put system in such state.

@yuliiamir
Copy link
Collaborator

After deactivating/activating stats/flowmonitoring/opentsdb topologies, the required statistics are available.
Details:

stats 
shutdown -> November 15th 2023, 18:28:41.336
Received lifecycle event LifecycleEvent(signal=SHUTDOWN, uuid=d1f669b7-888f-4541-bd53-e9abcc0e22e4, messageId=2)
start -> November 15th 2023, 18:28:57.807
Component stats with id blue got start event with known UUID 56315bf7-06f4-4f11-99d9-9403e76f9997. Active is 7 now

flowmonitoring
shutdown -> November 15th 2023, 18:35:37.167
Received lifecycle event LifecycleEvent(signal=SHUTDOWN, uuid=04d8f676-67ba-4235-8c83-05e84d43ce05, messageId=2)
start -> November 15th 2023, 18:37:13.590 
Component flowmonitoring with id blue got start event with known UUID bbc3b2f2-6af4-4839-95b4-2aeddb74d58b. Active is 7 now

otsdb 
shutdown -> November 15th 2023, 18:36:59.408
Component opentsdb with id blue received signal SHUTDOWN from zookeeper. Sending event LifecycleEvent(signal=SHUTDOWN, uuid=e10f53f3-06d9-4f96-b6d4-44c5d6cbc3f8, messageId=2)
start -> November 15th 2023, 18:37:13.648
Component opentsdb with id blue got start event with known UUID aff619f1-6cf0-43a6-b3a2-9fae5add029b. Active is 2 now

OTSDBGraph

VictoriaGraph

FlowRttFlowMonitoring

@pablomuri pablomuri merged commit 7123c27 into develop Nov 16, 2023
2 checks passed
@pablomuri pablomuri deleted the fix/clear_cahces branch November 16, 2023 10:54
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.

5 participants