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

Caching implementation of EventKeyFactory #4843

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

dlvenable
Copy link
Member

Description

Update the EventKeyFactory to support caching of EventKey objects across plugins and within them.

Also adds a configuration to data-prepper-config.yaml:

event:
  maximum_cached_keys: 512

Issues Resolved

N/A

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…configurable number of EventKeys.

Signed-off-by: David Venable <[email protected]>
@dlvenable
Copy link
Member Author

See #4842 for how this can help with performance for parse_json.

graytaylor0
graytaylor0 previously approved these changes Aug 16, 2024
@Inject
CachingEventKeyFactory(final EventKeyFactory delegateEventKeyFactory, final EventConfiguration eventConfiguration) {
this.delegateEventKeyFactory = delegateEventKeyFactory;
cache = Caffeine.newBuilder()
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I was not familiar with this library before

Copy link
Member Author

Choose a reason for hiding this comment

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

Caffeine is preferred over the Guava cache. And we actually have a Checkstyle to prevent use of the Guava cache.

@graytaylor0
Copy link
Member

I see the build is failing

@graytaylor0 graytaylor0 dismissed their stale review August 16, 2024 22:39

Build is failing

…lways remove items when we need.

Signed-off-by: David Venable <[email protected]>
…tion context for a couple of reasons: 1. Allow for skipping the CachingEventKeyFactory if not needed; 2. Help it run better in the data-prepper-test-event project.

Signed-off-by: David Venable <[email protected]>
Signed-off-by: David Venable <[email protected]>
Signed-off-by: David Venable <[email protected]>
final String key = UUID.randomUUID().toString();
final EventKey eventKey = mock(EventKey.class);

when(innerEventKeyFactory.createEventKey(key, EventKeyFactory.EventAction.ALL)).thenReturn(eventKey);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this test testing? Since we are returning via when it will be same instance, right? Isn't it better to use the real createEventKey()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Overall, this test is verifying the behavior of createEventKey(String) in the CachingEventKeyFactory. The interface has a default implementation for createEventKey(final String key):

default EventKey createEventKey(final String key) {
return createEventKey(key, EventAction.ALL);
}

The CachingEventKeyFactory actually uses this default implementation, which then calls the custom implementation for caching.

The real implementations are being used in this test. Please note that I am not using a spy on the object under test. On this line, I have mocked the inner object. This is the same as on line 55 above. The difference is that because of the default implementation, the call comes in providing EventAction.ALL.

@dlvenable dlvenable added this to the v2.10 milestone Oct 7, 2024
@dlvenable dlvenable merged commit 55c01b8 into opensearch-project:main Oct 7, 2024
49 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants