Conversation
Kvs::open() always read snapshot 0, so a KVS whose current generation was missing or corrupted could not be initialized even when older snapshots were present and intact. The redundancy the component pays for was unreachable through the API. Add an optional snapshot ID to Kvs::open() and a matching KvsBuilder::snapshot_id() option, both defaulting to snapshot 0 so existing callers are unaffected. No implicit fallback is performed. If the requested snapshot is unavailable the existing need_kvs flag decides between an error and an empty KVS, so an application is never silently started on data of unknown age. Signed-off-by: atarekra <ahmed.tarek-ramadan@valeo.com>
atarekra
requested review from
PandaeDo,
antonkri,
umaucher and
vinodreddy-g
as code owners
August 31, 2026 07:44
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 |
| OpenNeedKvs need_kvs, | ||
| const std::string&& dir); | ||
| const std::string&& dir, | ||
| const SnapshotId& snapshot_id = SnapshotId(0)); |
Contributor
There was a problem hiding this comment.
snapshotId already initialized on builder constructor , why do you reintialize it here again ? most probably to avoid updating testing.
This branch was successfully deployed
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
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.
Closes #237
The problem
The KVS keeps older snapshots so that a damaged current file isn't fatal, but
Kvs::open()only ever read snapshot 0 — the path was built with a hardcoded"_0". If that file was missing or corrupt, the KVS refused to initialize evenwhen
_1,_2and_3were present and intact. The recovery data existed andnothing in the API could reach it.
The change
Kvs::open()takes an optional snapshot ID, defaulting toSnapshotId(0), andKvsBuildergets a matchingsnapshot_id()option:The default keeps every existing caller working unchanged.
I went with the first option from the issue (explicit builder parameter) rather
than the automatic retry loop, for the reason given in the issue itself: an
implicit fallback means the application starts up on data of unknown age with no
signal that anything went wrong. So a missing snapshot still fails, and
need_kvs_flag()decides whether that's an error or an empty KVS. There's a testthat pins this down deliberately, so the fallback doesn't get added later by
accident.
Tests
7 new tests: the default path still reads snapshot 0; opening from an explicit
snapshot works with
_0deleted; a missing snapshot fails rather than fallingback; Optional semantics apply to the requested ID; and the full recovery story
(open an older generation, flush, confirm it becomes the current KVS).
Notes
kvs_builder.rsalso hardcodesSnapshotId(0).