Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Dynamic trailing computation #714

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

marco6
Copy link
Collaborator

@marco6 marco6 commented Oct 18, 2024

This PR adds a non-default strategy to dynamically compute the amount of entries to keep in memory based on the size of the snapshot.

The problem with using a fixed number is that each entry has a different size (depending on the number of pages written for each transaction). As such, the memory used (and then streamed to a lagging follower) might be way bigger than the size of the snapshot itself (even by order of magnitude).

@marco6
Copy link
Collaborator Author

marco6 commented Oct 18, 2024

This PR lacks testing for now.

@marco6 marco6 marked this pull request as draft October 18, 2024 09:15
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 80.51948% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.02%. Comparing base (36eac86) to head (3b87142).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
src/raft/replication.c 53.33% 2 Missing and 5 partials ⚠️
src/server.c 70.00% 3 Missing ⚠️
src/raft/raft.c 75.00% 2 Missing ⚠️
test/raft/integration/test_snapshot.c 94.11% 0 Missing and 2 partials ⚠️
src/raft/log.c 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #714   +/-   ##
=======================================
  Coverage   74.02%   74.02%           
=======================================
  Files         192      192           
  Lines       27827    27899   +72     
  Branches     2797     2802    +5     
=======================================
+ Hits        20598    20653   +55     
- Misses       4906     4914    +8     
- Partials     2323     2332    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@letFunny
Copy link
Contributor

letFunny commented Nov 1, 2024

Thanks Marco for this PR, it looks very good to me, no comments so far. For the testing part of the story, we have the snapshot tests at test/raft/integration/test_snapshot.c so we could probably add this alternate strategy as a parameter for the existing tests there (where it makes sense). There are other tests that might be applicable such as the replication tests (test_replication.c).

We should also add a new test for the dynamic strategy asserting that the entries in the snapshot match the calculation for the expected size. Actually I was going to suggest changing existing tests to use both strategies by abstracting it away but I see that most of them use a threshold of 1 so it will probably require changing all of them, so adding a new one seems easier. I would suggest adding the tests in the unit tests for replication and in the integration tests, either in the replication_test or in snapshot_test.

Lastly, do not look at the tests under test/raft/unit/test_snapshot.c because those are for the new snapshot procedure that was never released.

@marco6
Copy link
Collaborator Author

marco6 commented Feb 5, 2025

After the conversation I had with @just-now, I changed slightly the strategy so that the computed trailing always sits in the range [threshold, trailing] so that the previous snapshot is still useful.

I also added some tests which hopefully cover the functionality now. I'm marking this as ready.

@marco6 marco6 marked this pull request as ready for review February 5, 2025 14:56
Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

Looks very good. Just minor nit-picks.

src/raft.h Outdated Show resolved Hide resolved
src/raft/raft.c Outdated Show resolved Hide resolved
src/raft/replication.c Outdated Show resolved Hide resolved
src/raft/replication.c Outdated Show resolved Hide resolved
src/raft/replication.c Outdated Show resolved Hide resolved
src/raft/replication.c Show resolved Hide resolved
@marco6 marco6 force-pushed the dynamic-trailing-computation branch 2 times, most recently from 38d61c5 to 83de83e Compare February 7, 2025 15:14
Copy link
Contributor

@just-now just-now left a comment

Choose a reason for hiding this comment

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

Looks good. Merge it (preferably on Monday).

@marco6 marco6 force-pushed the dynamic-trailing-computation branch from 37bff78 to 3b87142 Compare February 10, 2025 08:38
@marco6 marco6 merged commit 4c49d22 into canonical:master Feb 10, 2025
10 of 11 checks passed
@marco6 marco6 deleted the dynamic-trailing-computation branch February 10, 2025 08:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants