impl: KVS Instance state registry [cpp] by karolnowak · Pull Request #538 · eclipse-score/persistency

karolnowak · 2026-10-20T09:38:40Z

Problem

The existing [cpp] impl allows to load the same instance from disk many times.
After loading, each KVS operates on its own non-shared entries.
This can affect data integrity when two KVSs, which refer to the same instance, save their entries on disk.

KvsBuilder builder(instance_id=1);
builder.build(); -> KVS with its own entries.*
builder.build(); -> another KVS with its own entries.*
builder.build(); -> yet another KVS with its own entries.*

*Even though all KVSs refer to the same instance.

Solution

A registry to share the same state of a given KVS Instance.

Note

At the moment, the registry implements the Singleton pattern and is hidden from the lib user.
- I don't think it is the best approach, however, the idea was to be aligned with the Rust impl.
I believe that the registry should be rewritten so that:
- It is available for the lib user.
- The lib user can create its own instance of the registry - No issue with the order of destruction.
- The lib user can specify how many kvs instances the registry can hold.

KvsRegistry registry(instance_count, ...);
registry.getOrOpen(params); -> `Kvs`

github-actions · 2026-10-20T09:42:30Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Starting local Bazel server (8.7.0) and connecting to it...
INFO: Invocation ID: 7d7cbc33-7a87-4a77-adcb-c08054531d69
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 2 packages loaded
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Analyzing: target //:license-check (3 packages loaded)
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)

Analyzing: target //:license-check (94 packages loaded, 14 targets configured)

Analyzing: target //:license-check (99 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (196 packages loaded, 3948 targets configured)

Analyzing: target //:license-check (202 packages loaded, 11366 targets configured)

Analyzing: target //:license-check (203 packages loaded, 11377 targets configured)

Analyzing: target //:license-check (208 packages loaded, 11785 targets configured)

Analyzing: target //:license-check (209 packages loaded, 15886 targets configured)

Analyzing: target //:license-check (212 packages loaded, 17774 targets configured)

Analyzing: target //:license-check (212 packages loaded, 17774 targets configured)

INFO: Analyzed target //:license-check (213 packages loaded, 17903 targets configured).
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 50 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/tools/formatted.txt
[14 / 16] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
INFO: Found 1 target...
Target //tools:license.check.license_check up-to-date:
  bazel-bin/tools/license.check.license_check
  bazel-bin/tools/license.check.license_check.jar
INFO: Elapsed time: 42.606s, Critical Path: 2.25s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/tools/license.check.license_check tools/formatted.txt <args omitted>
[main] INFO Querying Eclipse Foundation for license data for 42 items.
[main] INFO Found 32 items.
[main] INFO Querying ClearlyDefined for license data for 10 items.
[main] INFO Found 10 items.
[main] INFO License information could not be automatically verified for the following content:
[main] INFO 
[main] INFO crate/cratesio/-/lazy_static/1.5.1
[main] INFO 
[main] INFO This content is either not correctly mapped by the system, or requires review.

github-actions · 2026-10-20T09:53:02Z

Documentation preview for this pull request is available at:
pr-538: https://eclipse-score.github.io/persistency/pr-538/

Ahmed-Elsaka-JC · 2026-10-20T22:58:05Z

+    KvsStateRegistry(KvsStateRegistry&&) = delete;
+
+    KvsStateRegistry& operator=(const KvsStateRegistry&) = delete;
+    KvsStateRegistry& operator=(KvsStateRegistry&&) = delete;


please let us try to discuss this approach coming meeting .
based on discussion we can decide if we will proceed with this approach or another approach.

arkjedrz

Review is probably not exhaustive, but discussions might be needed at this points.

arkjedrz · 2026-10-23T07:31:08Z

    /* Create Test Data*/
-    kvs_b.kvs_.insert({"test_kvs", KvsValue(42.0)});
-    kvs_b.default_values_.insert({"test_default", KvsValue(true)});
+    kvs_b.state_->current_values_.insert({"test_kvs", KvsValue(42.0)});


Not in scope here, but this could be avoided with #574

A registry to share the same state of a given KVS Instance. It avoids loading a given KVS Instance from disk again which could otherwise lead to the use of different versions of the same instance. Signed-off-by: Karol Nowak <Karol.Novak.Ext@qorix.ai>

arkjedrz

Mostly okay for what it can be right now.
E.g., backend would be welcome in future change. Unit tests cause a lot of changes due to pending #593. Some changes overlap with #490.

arkjedrz · 2026-10-27T12:46:00Z


 namespace score::mw::per::kvs
 {
+/// @brief Forward declaration.


Please model changes in here after #490.

arkjedrz · 2026-10-27T12:49:44Z

-    const std::string directory = directory_.empty() ? std::string{"./"} : directory_;
+    const std::string directory{directory_.empty() ? std::string{"./"} : directory_};
+
+    const std::size_t max_snapshots_clmaped{std::clamp(max_snapshots_, 0ul, KVS_MAX_SNAPSHOTS_UPPER_BOUND)};


clmaped -> clamped?

arkjedrz · 2026-10-27T13:00:35Z

+{
+    /// Protects access to the shared state.
+    std::mutex mutex;
+    /// Instance id that has been specified during creation.


Nitpick - ID instead of id.

arkjedrz · 2026-10-27T13:02:31Z

+class KvsInstanceState;
+
+/// @brief Maximum number or instances.
+constexpr std::size_t KVS_MAX_INSTANCE_COUNT{8ul};


Nitpick - align with Rust to 10.
IMO this doesn't matter much, since other storage type should be used.

arkjedrz · 2026-10-27T13:04:50Z

+class KvsInstanceState;
+
+/// @brief Maximum number or instances.
+constexpr std::size_t KVS_MAX_INSTANCE_COUNT{8ul};


Shouldn't that be a kvsbuilder.hpp constant?

arkjedrz · 2026-10-27T13:10:44Z

+    std::mutex mutex{};
+    /// Array.
+    Array array{};
+    /// Entry count.


Drop the size, map instance ID directly to the array.
E.g., if requested KVS for instance ID == 3, then occupy this slot. If requested KVS for instance ID then report error that it cannot be made.

karolnowak requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners October 20, 2026 09:38

karolnowak deployed to workflow-approval October 20, 2026 09:38 — with GitHub Actions Active

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 3168f21 to f6be05e Compare October 20, 2026 09:43

karolnowak deployed to workflow-approval October 20, 2026 09:44 — with GitHub Actions Active

vinodreddy-g requested a review from arkjedrz October 20, 2026 11:28

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from f6be05e to cc7e450 Compare October 20, 2026 13:22

karolnowak deployed to workflow-approval October 20, 2026 13:22 — with GitHub Actions Active

Ahmed-Elsaka-JC suggested changes Oct 20, 2026

View reviewed changes

arkjedrz requested changes Oct 23, 2026

View reviewed changes

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from cc7e450 to e734e41 Compare October 26, 2026 10:45

karolnowak had a problem deploying to workflow-approval October 26, 2026 10:45 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from e734e41 to 614a402 Compare October 26, 2026 13:31

karolnowak had a problem deploying to workflow-approval October 26, 2026 13:31 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 614a402 to 62b64f8 Compare October 26, 2026 14:18

karolnowak deployed to workflow-approval October 26, 2026 14:18 — with GitHub Actions Active

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 62b64f8 to e30a0d8 Compare October 26, 2026 15:57

karolnowak had a problem deploying to workflow-approval October 26, 2026 15:57 — with GitHub Actions Error

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from e30a0d8 to 4ee69cc Compare October 26, 2026 16:13

karolnowak deployed to workflow-approval October 26, 2026 16:13 — with GitHub Actions Active

karolnowak force-pushed the karolnowak-kvs-instatnce-state-registry branch from 4ee69cc to ed50e69 Compare October 27, 2026 08:48

karolnowak deployed to workflow-approval October 27, 2026 08:48 — with GitHub Actions Active

arkjedrz requested changes Oct 27, 2026

View reviewed changes

Conversation

karolnowak commented Oct 20, 2026

Problem

Solution

Note

Uh oh!

github-actions Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

Ahmed-Elsaka-JC Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arkjedrz Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Oct 20, 2026 •

edited

Loading

Ahmed-Elsaka-JC Oct 20, 2026 •

edited

Loading