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

feat: consolidate event processing and optimize application performance #23

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

jboix
Copy link
Contributor

@jboix jboix commented Jan 27, 2025

Description

Resolves #22 by updating the event processing system to improve efficiency.

Changes Made

  • Switched from Reactor to Kotlin Flow for event handling to improve readability.
  • Events are now saved in batch, reducing concurrency and improving performance.
  • Removed reconciliation logic with a more aggressive in memory cache approach.
  • Replaced Caffeine with a simple LinkedHashMap cache to support LRU eviction policy.
  • Added configuration parameters to adjust the size of the cache, buffer and batch chunk size. These
    properties can also be modified with environment variables.
  • Removed actuator: benchmarks are now log, reducing memory consumption and eliminating the need
    for starting a server.
  • Updated dependencies to the latest compatible versions.
  • Replaced dynamic mapping in OpenSearch with a handmade mapping for document fields. This change
    reduces storage requirements, and eliminates unnecessary fields, making queries more logical and
    efficient.

Checklist

  • I have followed the project's style and contribution guidelines.
  • I have performed a self-review of my own changes.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.

@jboix jboix self-assigned this Jan 27, 2025
Copy link

github-actions bot commented Jan 27, 2025

Coverage Report

Overall Project 59.7% -31.84% 🍏
Files changed 26.45% 🍏

File Coverage
EventDispatcherClientConfiguration.kt 100% 🍏
AliasSetupTask.kt 100% 🍏
LRUCache.kt 100% 🍏
BenchmarkScheduledLogger.kt 100% 🍏
EventRequest.kt 98.51% 🍏
OpenSearchSetupService.kt 96.7% 🍏
OpenSearchConfigurationProperties.kt 90.14% 🍏
OpenSearchConfiguration.kt 78.38% -13.51% 🍏
StatsTracker.kt 35.85% -64.15% 🍏
RetryProperties.kt 29.31% -70.69% 🍏
TimeTracker.kt 27.06% -72.94% 🍏
LoggingExtensions.kt 23.7% -18.52% 🍏
PillarboxDataTransferApplication.kt 18.18% 🍏
EventDispatcherClient.kt 10.05% -84.69% 🍏
FlowUtils.kt 0% 🍏
DataTransferApplicationRunner.kt 0% -82.42% 🍏
MovingAverageCalculator.kt 0% 🍏
ElasticsearchRepositoryExtensions.kt 0% 🍏

@jboix jboix force-pushed the feat/remove-actuator branch 2 times, most recently from 9994777 to f190a11 Compare January 27, 2025 11:02
@jboix jboix requested a review from MGaetan89 January 27, 2025 11:57
@jboix jboix marked this pull request as ready for review January 27, 2025 11:57
@ben-manes
Copy link

I'd recommend using Guava's Cache instead of Ehcache if you require LRU. I may be biased, but my opinion is that library is not safe for production users due to a trivially exploitable denial of service attack. Both have bugs and are inactive.

fwiw - Caffeine's eviction policy is adaptive, so it starts biased towards frequency and adjusts towards recency based on the observed workload. It does not provide any guarantees if you rely on LRU semantics for correct application behavior, as a more important constraint than the hit rate. If that is the case, then I think making the logic more explicit about the LRU requirement (e.g. via a LinkedHashMap or similar) is better for maintainability.

@jboix jboix force-pushed the feat/remove-actuator branch from f190a11 to fdb4eaf Compare January 28, 2025 14:50
@jboix
Copy link
Contributor Author

jboix commented Jan 28, 2025

@ben-manes Thank you for the feedback! I've used Caffeine before and really appreciate its simplicity. However, for our particular use case, we require a strict LRU eviction policy.

After evaluating your comment, I reconsidered our previous implementation and decided to drop EhCache and add a simple LRU cache using LinkedHashMap. This approach is straightforward and avoids introducing an additional dependency, which we value.

The event processing system has been updated to improve readability and efficiency:

- Switched from Reactor to Kotlin Flow for event handling to improve readability.
- Events are now saved in batch, reducing concurrency and improving performance.
- Removed reconciliation logic with a more aggressive in memory cache approach.
- Replaced Caffeine with a simple LinkedHashMap cache to support LRU eviction policy.
- Added configuration parameters to adjust the size of the cache, buffer and batch chunk size. These
  properties can also be modified with environment variables.
- Removed actuator: benchmarks are now log, reducing memory consumption and eliminating the need
  for starting a server.
- Updated dependencies to the latest compatible versions.
- Replaced dynamic mapping in OpenSearch with a handmade mapping for document fields. This change
  reduces storage requirements, and eliminates unnecessary fields, making queries more logical and
  efficient.

Co-authored-by: Gaëtan Muller <[email protected]>
@jboix jboix force-pushed the feat/remove-actuator branch from fdb4eaf to 4b202ca Compare January 29, 2025 10:22
@jboix jboix added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit 143d2e0 Jan 29, 2025
1 check passed
@jboix jboix deleted the feat/remove-actuator branch January 29, 2025 10:39
Copy link

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event consolidation
4 participants