-
Notifications
You must be signed in to change notification settings - Fork 569
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
Improve the traversing of snapshot files #12575
Labels
kind/feature
Categorizes an issue or PR as a feature, i.e. new behavior
version:8.1.13
Marks an issue as being completely or in parts released in 8.1.13
version:8.2.5
Marks an issue as being completely or in parts released in 8.2.5
version:8.3.0-alpha2
Marks an issue as being completely or in parts released in 8.3.0-alpha2
version:8.3.0
Marks an issue as being completely or in parts released in 8.3.0
Comments
Merged
14 tasks
@Zelldon Hello 👋 |
Looks like @deepthidevaki will look into your PR, thanks for providing it :) |
zeebe-bors-camunda bot
added a commit
that referenced
this issue
May 4, 2023
12576: refactor(snapshots): Replace `Stream.toList` and the for each cycle to `Stream.forEachOrdered` r=deepthidevaki a=aivinog1 ## Description <!-- Please explain the changes you made here. --> I replaced the collection with a list and the for each cycle to the `Stream.forEactOrdered` call. It should reduce memory footprint and possibly improve latency, especially on a large number of snapshot files. ## Related issues <!-- Which issues are closed by this PR or are related --> closes #12575 Co-authored-by: Alexey Vinogradov <vinogradov.a.i.93@gmail.com>
zeebe-bors-camunda bot
added a commit
that referenced
this issue
May 4, 2023
12662: [Backport stable/8.1] fix: do not retake backup if it already exists r=deepthidevaki a=deepthidevaki Backport of #12626 to stable/8.1. relates to #12623 12665: [Backport stable/8.1] refactor(snapshots): Replace `Stream.toList` and the for each cycle to `Stream.forEachOrdered` r=deepthidevaki a=backport-action # Description Backport of #12576 to `stable/8.1`. relates to #12575 Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com> Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@users.noreply.github.com> Co-authored-by: Alexey Vinogradov <vinogradov.a.i.93@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
kind/feature
Categorizes an issue or PR as a feature, i.e. new behavior
version:8.1.13
Marks an issue as being completely or in parts released in 8.1.13
version:8.2.5
Marks an issue as being completely or in parts released in 8.2.5
version:8.3.0-alpha2
Marks an issue as being completely or in parts released in 8.3.0-alpha2
version:8.3.0
Marks an issue as being completely or in parts released in 8.3.0
Is your feature request related to a problem? Please describe.
I think that it is not a good idea to collect snapshot files in the list.
Check it here: https://github.com/camunda/zeebe/blob/02d1df8ac1b6f5484f1e9d4c1f19c8a5712176b1/snapshot/src/main/java/io/camunda/zeebe/snapshots/impl/SnapshotChecksum.java#L41.
Because, after a while, the amount of snapshot files should grow. So, we could instead call the
.forEachOrdered
method to calculate the snapshot. I will provide the PR soon to see this in action and benchmark this.Describe the solution you'd like
We should call
Stream.forEachOrdered
instead of collecting snapshotsFile
s in the list.Describe alternatives you've considered
We could use
Stream.forEach
but as I can understand the order is important, so we shouldn't do this.The text was updated successfully, but these errors were encountered: