Skip to content

feat: make KVS max snapshot count configurable #383

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.

Sign up for GitHub

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

Open
atarekra wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: make KVS max snapshot count configurable #383

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be391de
feat: make KVS max snapshot count configurable
atarekra Aug 21, 2026
9b1bcfc
Merge branch 'main' into kvs-configurable-max-snapshots
atarekra Aug 30, 2026
bacc540
fix: move comma before trailing comment in KvsBuilder
atarekra Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions score/kvs/kvs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ namespace score::mw::per::kvs

/*********************** KVS Implementation *********************/
Kvs::Kvs()
: filesystem(std::make_unique<score::filesystem::Filesystem>(
: max_snapshots(KVS_DEFAULT_MAX_SNAPSHOTS),
filesystem(std::make_unique<score::filesystem::Filesystem>(
score::filesystem::FilesystemFactory{}.CreateInstance())) /* Create Filesystem instance, noexcept call */
,
parser(std::make_unique<score::json::JsonParser>()),
Expand All @@ -42,6 +43,7 @@ Kvs::Kvs()

Kvs::Kvs(Kvs&& other) noexcept
: filename_prefix(std::move(other.filename_prefix)),
max_snapshots(other.max_snapshots),
filesystem(std::move(other.filesystem)),
parser(std::move(other.parser)) /* Not absolutely necessary, because a new JSON writer/parser
object would also be okay*/
Expand All @@ -67,6 +69,7 @@ Kvs& Kvs::operator=(Kvs&& other) noexcept
}
default_values.clear();
filename_prefix = std::move(other.filename_prefix);
max_snapshots = other.max_snapshots;

{
std::lock_guard<std::mutex> lock_other(other.kvs_mutex);
Expand Down Expand Up @@ -218,7 +221,8 @@ score::Result<std::unordered_map<string, KvsValue>> Kvs::open_json(const score::
score::Result<Kvs> Kvs::open(const InstanceId& instance_id,
OpenNeedDefaults need_defaults,
OpenNeedKvs need_kvs,
const std::string&& dir)
const std::string&& dir,
std::size_t snapshot_max_count)
{
score::Result<Kvs> result =
score::MakeUnexpected(ErrorCode::UnmappedError); /* Redundant initialization needed, since Resul<KVS> would call
Expand Down Expand Up @@ -251,8 +255,9 @@ score::Result<Kvs> Kvs::open(const InstanceId& instance_id,
kvs.kvs = std::move(kvs_res.value());
kvs.default_values = std::move(default_res.value());
kvs.filename_prefix = filename_prefix;
kvs.max_snapshots = snapshot_max_count;
kvs.logger->LogInfo() << "opened KVS: instance '" << instance_id.id << "'";
kvs.logger->LogInfo() << "max snapshot count: " << KVS_MAX_SNAPSHOTS;
kvs.logger->LogInfo() << "max snapshot count: " << snapshot_max_count;
result = std::move(kvs);
}
}
Expand Down Expand Up @@ -618,7 +623,7 @@ score::Result<size_t> Kvs::snapshot_count() const
score::Result<size_t> result = score::MakeUnexpected(ErrorCode::UnmappedError);
size_t count = 0;
bool error = false;
for (size_t idx = 0; idx < KVS_MAX_SNAPSHOTS; ++idx)
for (size_t idx = 0; idx < max_snapshots; ++idx)
{
const score::filesystem::Path fname = filename_prefix.Native() + "_" + to_string(idx) + ".json";
const auto fname_exists_res = filesystem->standard->Exists(fname);
Expand Down Expand Up @@ -651,7 +656,7 @@ score::Result<size_t> Kvs::snapshot_count() const
/* Retrieve the max snapshot count*/
size_t Kvs::snapshot_max_count() const
{
return KVS_MAX_SNAPSHOTS;
return max_snapshots;
}

/* Rotate Snapshots */
Expand All @@ -662,7 +667,7 @@ score::ResultBlank Kvs::snapshot_rotate()
if (lock.owns_lock())
{
bool error = false;
for (size_t idx = KVS_MAX_SNAPSHOTS; idx > 0; --idx)
for (size_t idx = max_snapshots; idx > 0; --idx)
{
score::filesystem::Path hash_old = filename_prefix.Native() + "_" + to_string(idx - 1) + ".hash";
score::filesystem::Path hash_new = filename_prefix.Native() + "_" + to_string(idx) + ".hash";
Expand Down
25 changes: 21 additions & 4 deletions score/kvs/kvs.hpp
View file
Open in desktop

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@
#include <unordered_map>
#include <vector>

#define KVS_MAX_SNAPSHOTS 3

namespace score::mw::per::kvs
{

/* comp_req__kvs__constraints: Compile-time default for the maximum number of snapshots.
comp_req__kvs__snapshot_max_num: The value is only a default. It can be overridden per
instance at init-time via KvsBuilder::snapshot_max_count(). */
constexpr std::size_t KVS_DEFAULT_MAX_SNAPSHOTS = 3U;

struct InstanceId
{
size_t id;
Expand Down Expand Up @@ -161,6 +164,10 @@ class Kvs final
* - OpenNeedKvs::Optional: An empty KVS will be used if no KVS exists.
* @param dir The directory path where the KVS files are located. It is passed as an rvalue
* reference to avoid unnecessary copying. Use "" or "." for the current directory.
* @param snapshot_max_count The maximum number of snapshots this instance maintains.
* Defaults to KVS_DEFAULT_MAX_SNAPSHOTS.
* A value of 0 keeps no previous generation; the current
* KVS data is still persisted by flush().
* @return A Result object containing either:
* - A Kvs object if the operation is successful.
* - An ErrorCode if an error occurs during the operation.
Expand All @@ -172,7 +179,8 @@ class Kvs final
static score::Result<Kvs> open(const InstanceId& instance_id,
OpenNeedDefaults need_defaults,
OpenNeedKvs need_kvs,
const std::string&& dir);
const std::string&& dir,
std::size_t snapshot_max_count = KVS_DEFAULT_MAX_SNAPSHOTS);

/**
* @brief Resets a key-value-storage to its initial state
Expand Down Expand Up @@ -287,6 +295,10 @@ class Kvs final
* @brief Flushes the key-value store, ensuring that all pending changes
* are written to the underlying storage.
*
* The current KVS data is always written. A configured maximum of 0 snapshots
* only means that no previous generation is kept: rotation does nothing and
* snapshot_count() stays 0, but the current data is still persisted.
*
* @return A score::Result object that indicates the success or failure of the operation.
* - On success: Returns a blank score::Result.
* - On failure: Returns an ErrorCode describing the error.
Expand All @@ -306,7 +318,9 @@ class Kvs final
* @brief Retrieves the maximum number of snapshots that can be stored.
*
* This function returns the upper limit on the number of snapshots
* that the key-value store can maintain at any given time.
* that the key-value store can maintain at any given time. The limit is
* configured per instance via KvsBuilder::snapshot_max_count() and defaults
* to KVS_DEFAULT_MAX_SNAPSHOTS.
*
* @return The maximum count of snapshots as a size_t value.
*/
Expand Down Expand Up @@ -366,6 +380,9 @@ class Kvs final
/* Filename prefix */
score::filesystem::Path filename_prefix;

/* comp_req__kvs__snapshot_max_num: Maximum number of snapshots maintained by this instance */
std::size_t max_snapshots;

/* Filesystem handling */
std::unique_ptr<score::filesystem::Filesystem> filesystem;

Expand Down
11 changes: 10 additions & 1 deletion score/kvs/kvsbuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ KvsBuilder::KvsBuilder(const InstanceId& instance_id)
need_defaults(false),
need_kvs(false),
directory("./data_folder/") /* Default Directory */
,
Comment thread
atarekra marked this conversation as resolved.
Outdated
max_snapshots(KVS_DEFAULT_MAX_SNAPSHOTS)
{
}

Expand All @@ -42,6 +44,12 @@ KvsBuilder& KvsBuilder::dir(std::string&& dir_path)
return *this;
}

KvsBuilder& KvsBuilder::snapshot_max_count(std::size_t max_count)
{
this->max_snapshots = max_count;

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

check if maximum snapshot >=3 set it to 3

return *this;
}

score::Result<Kvs> KvsBuilder::build()
{
score::Result<Kvs> result = score::MakeUnexpected(ErrorCode::UnmappedError);
Expand All @@ -55,7 +63,8 @@ score::Result<Kvs> KvsBuilder::build()
result = Kvs::open(instance_id,
need_defaults ? OpenNeedDefaults::Required : OpenNeedDefaults::Optional,
need_kvs ? OpenNeedKvs::Required : OpenNeedKvs::Optional,
std::move(directory));
std::move(directory),
max_snapshots);

return result;
}
Expand Down
22 changes: 18 additions & 4 deletions score/kvs/kvsbuilder.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ class KvsBuilder final
*/
KvsBuilder& dir(std::string&& dir_path);

/**
* @brief Specify the maximum number of snapshots the KVS maintains.
*
* comp_req__kvs__snapshot_max_num: The maximum number of snapshots is configurable
* per instance. When this option is not used, KVS_DEFAULT_MAX_SNAPSHOTS is applied.
*
* @param max_count Maximum number of snapshots. A value of 0 keeps no previous
* generation; flush() still persists the current KVS data.
*
* @return Reference to this builder (for chaining).
*/
KvsBuilder& snapshot_max_count(std::size_t max_count);

/**
* @brief Builds and opens the Kvs instance with the configured options.
*
Expand All @@ -99,10 +112,11 @@ class KvsBuilder final
score::Result<Kvs> build();

private:
InstanceId instance_id; ///< ID of the KVS instance
bool need_defaults; ///< Whether default values are required
bool need_kvs; ///< Whether an existing KVS is required
std::string directory; ///< Directory where to store the KVS Files
InstanceId instance_id; ///< ID of the KVS instance
bool need_defaults; ///< Whether default values are required
bool need_kvs; ///< Whether an existing KVS is required
std::string directory; ///< Directory where to store the KVS Files
std::size_t max_snapshots; ///< Maximum number of snapshots to maintain
};

} /* namespace score::mw::per::kvs */
Expand Down
12 changes: 1 addition & 11 deletions score/kvs/tests/test_cases/tests/test_cit_snapshots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,7 @@ def test_ok(
results: ScenarioResult,
logs_info_level: LogContainer,
snapshot_max_count: int,
version: str,
):
if version == "cpp" and snapshot_max_count in [0, 1, 3, 10]:
pytest.xfail(
reason="https://github.com/eclipse-score/persistency/issues/108",
)
assert results.return_code == ResultCode.SUCCESS

count = test_config["count"]
Expand Down Expand Up @@ -114,7 +109,7 @@ def test_config(self, temp_dir: Path, snapshot_max_count: int) -> dict[str, Any]
)
@pytest.mark.parametrize("snapshot_max_count", [0, 1, 3, 10], scope="class")
class TestSnapshotMaxCount(MaxSnapshotsScenario):
"""Verifies that the maximum number of snapshots is a constant value."""
"""Verifies that the KVS instance reports the configured maximum number of snapshots."""

@pytest.fixture(scope="class")
def scenario_name(self) -> str:
Expand All @@ -135,12 +130,7 @@ def test_ok(
results: ScenarioResult,
logs_info_level: LogContainer,
snapshot_max_count: int,
version: str,
):
if version == "cpp":
pytest.xfail(
reason="https://github.com/eclipse-score/persistency/issues/108",
)
assert results.return_code == ResultCode.SUCCESS
assert logs_info_level.find_log("max_count", value=snapshot_max_count) is not None

Expand Down
Loading
Loading