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

Add flag to disable kinesis-mock persistence #9871

Merged
merged 1 commit into from Dec 18, 2023

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Dec 14, 2023

Motivation

With recent kinesis performance tests, we have seen a significant performance degradation up to an out-of-memory error of kinesis-mock, especially when pushing over 50 entries with the maximum payload of 1024KB to a stream.
This behavior is way worse when the persistence is enabled when starting kinesis-mock. For now, to fix it for users not needing persistence, we need a flag to disable it.

In the future, we need to spend some work on increasing the performance and memory usage of the persistence of our kinesis implementation.

Changes

  • Add flag to disable the persistence mechanism for kinesis, which is currently active regardless of cloud pods or persistence being used on the localstack level.

… of larger payloads, until we find an upstream fix
@dfangl dfangl added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Dec 14, 2023
@dfangl dfangl requested a review from thrau as a code owner December 14, 2023 10:58
Copy link

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0      2 suites  ±0   3m 14s ⏱️ ±0s
381 tests ±0  331 ✔️ ±0    50 💤 ±0  0 ±0 
762 runs  ±0  662 ✔️ ±0  100 💤 ±0  0 ±0 

Results for commit 3644427. ± Comparison against base commit a77e4f8.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

that's a great catch! why not just use if config.PERSISTENCE though?

@dfangl
Copy link
Member Author

dfangl commented Dec 14, 2023

Because then cloud pods will not work anymore for kinesis, I think. For usual payload sizes currently persistence is not a problem, but for bigger ones it becomes one, so I felt like a separate variable to disable it made more sense.

@coveralls
Copy link

Coverage Status

coverage: 84.02% (-0.01%) from 84.031%
when pulling 3644427 on add-kinesis-persistence-flag
into a77e4f8 on master.

Copy link

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 11m 56s ⏱️ +18s
2 405 tests ±0  2 177 ✔️ ±0  228 💤 ±0  0 ±0 
2 406 runs  ±0  2 177 ✔️ ±0  229 💤 ±0  0 ±0 

Results for commit 3644427. ± Comparison against base commit a77e4f8.

@giograno
Copy link
Member

Correct, pods won't work if we don't start kinesis already with the correct env variables for persistence (there won't be any assets to persist). It would be very handy to have a sort of dump command to be called ad-hoc in kinesis mock 🤔

@dfangl dfangl requested a review from thrau December 15, 2023 09:02
@thrau thrau merged commit fb182b7 into master Dec 18, 2023
35 checks passed
@thrau thrau deleted the add-kinesis-persistence-flag branch December 18, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants