feat: make KVS max snapshot count configurable - #383
Open
atarekra wants to merge 3 commits into
Open
Conversation
The C++ KVS fixed the number of snapshots at three via the
KVS_MAX_SNAPSHOTS macro, so comp_req__kvs__snapshot_max_num
("configurable maximum number of snapshots") was not met and the
component integration tests xfailed all C++ boundary cases.
Replace the macro with a namespaced KVS_DEFAULT_MAX_SNAPSHOTS
constant and add KvsBuilder::snapshot_max_count(), forwarded through
Kvs::open() into a per-instance member used by snapshot counting,
rotation and the reported maximum. The value is bound at init-time
and immutable afterwards, since lowering it on a live store would
orphan snapshot files that rotation would never reclaim.
A configured maximum of zero keeps no previous generation but still
persists the current KVS data, matching the Rust backend, which
rotates (a no-op at zero) and then always saves snapshot 0.
Also fix SnapshotMaxCount::run reading a "count" field its scenario
config never supplies, which the blanket xfail had been hiding.
atarekra
requested review from
PandaeDo,
antonkri,
umaucher and
vinodreddy-g
as code owners
August 21, 2026 00:53
atarekra
requested a deployment
to
workflow-approval
August 21, 2026 00:53 — with 
GitHub Actions
Waiting
GitHub Actions
Waiting
atarekra
requested a deployment
to
workflow-approval
August 21, 2026 00:53 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
Contributor
|
@atarekra please resolve merge conflicts |
atarekra
requested a deployment
to
workflow-approval
August 30, 2026 08:51 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
atarekra
requested a deployment
to
workflow-approval
August 30, 2026 08:51 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
Contributor
Author
Solved |
arkjedrz
requested changes
Sep 1, 2026
arkjedrz
left a comment
arkjedrz
left a comment
Contributor
There was a problem hiding this comment.
Small issue, otherwise LGTM.
Signed-off-by: atarekra <ahmed.tarek-ramadan@valeo.com>
arkjedrz
approved these changes
Sep 2, 2026
|
|
||
| KvsBuilder& KvsBuilder::snapshot_max_count(std::size_t max_count) | ||
| { | ||
| this->max_snapshots = max_count; |
Contributor
There was a problem hiding this comment.
check if maximum snapshot >=3 set it to 3
umaucher
requested changes
Sep 10, 2026
umaucher
left a comment
umaucher
left a comment
Contributor
There was a problem hiding this comment.
see @Ahmed-Elsaka-JC commend
Contributor
There was a problem hiding this comment.
std::size_t snapshot_max_count = KVS_DEFAULT_MAX_SNAPSHOTS :
why do you use default value for snapshot_max_count again here ?? this default would be set but builder constructor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The C++ KVS fixed the number of snapshots at three via the KVS_MAX_SNAPSHOTS macro, so comp_req__kvs__snapshot_max_num ("configurable maximum number of snapshots") was not met and the component integration tests xfailed all C++ boundary cases.
Replace the macro with a namespaced KVS_DEFAULT_MAX_SNAPSHOTS constant and add KvsBuilder::snapshot_max_count(), forwarded through Kvs::open() into a per-instance member used by snapshot counting, rotation and the reported maximum. The value is bound at init-time and immutable afterwards, since lowering it on a live store would orphan snapshot files that rotation would never reclaim.
A configured maximum of zero keeps no previous generation but still persists the current KVS data, matching the Rust backend, which rotates (a no-op at zero) and then always saves snapshot 0.
Also fix SnapshotMaxCount::run reading a "count" field its scenario config never supplies, which the blanket xfail had been hiding.
Resolves #108