Snapshots feature refactor by AnnaPaolaMusio · Pull Request #292 · eclipse-score/persistency

AnnaPaolaMusio · 2026-05-26T16:36:13Z

The C++ implementation of the KVS module has been modified in terms of snapshot behavior.

Removal of snapshot_rotate
The snapshot_rotate function has been completely removed from the C++ implementation. Snapshot management is no longer automatic and requires an explicit call to the snapshot_create() function.
The flush() function no longer automatically creates snapshots.
In the previous behavior (still valid for RUST) flush() persisted the data and called snapshot_rotate() to automatically create a new snapshot. Now flush() now only persists the current state of the KVS to disk.
Implementation of snapshot_create API
The new snapshot_create() function has been implemented, allowing for the explicit creation of snapshots in the first available slot.
Implementation of snapshot_delete API
The snapshot_delete() function has been implemented to explicitly remove a specific snapshots by ID.
Update to snapshot_count and snapshot_restore.
A new component requirement has been introduced, according to the new behaviour of the snapshot_create.

github-actions · 2026-05-26T16:37:05Z

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]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 0660ef43-a66e-4091-b028-d4ef98e98455
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'rules_python', the root module requires module version rules_python@1.8.3, but got rules_python@1.8.5 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'buildifier_prebuilt', the root module requires module version buildifier_prebuilt@8.2.0.2, but got buildifier_prebuilt@8.5.1 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'aspect_rules_lint', the root module requires module version aspect_rules_lint@2.0.0, but got aspect_rules_lint@2.5.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Computing main repo mapping: 
Loading: 
Loading: 3 packages loaded
Loading: 3 packages loaded
    currently loading: 
Loading: 3 packages loaded
    currently loading: 
Loading: 3 packages loaded
    currently loading: 
Loading: 3 packages loaded
    currently loading: 
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)

Analyzing: target //:license-check (76 packages loaded, 10 targets configured)

Analyzing: target //:license-check (147 packages loaded, 4023 targets configured)

Analyzing: target //:license-check (154 packages loaded, 8078 targets configured)

Analyzing: target //:license-check (159 packages loaded, 8127 targets configured)

Analyzing: target //:license-check (159 packages loaded, 8127 targets configured)

Analyzing: target //:license-check (162 packages loaded, 10015 targets configured)

Analyzing: target //:license-check (162 packages loaded, 10015 targets configured)

INFO: Analyzed target //:license-check (164 packages loaded, 10265 targets configured).
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 66 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
[15 / 17] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 22.181s, Critical Path: 2.16s
INFO: 17 processes: 12 internal, 4 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 17 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

github-actions · 2026-05-27T11:34:49Z

The created documentation from the pull request is available at: docu-html

arkjedrz · 2026-06-16T10:33:32Z

-                result = write_json_data(buf);
-            }
+        {          
+            /* Write JSON Data */


Please add a comment here that it writes to a file with snapshot ID 0.
Add this information to flush method docs.

arkjedrz · 2026-06-16T10:38:31Z

    size_t count = 0;
-    bool error = false;
-    for (size_t idx = 0; idx < KVS_MAX_SNAPSHOTS; ++idx)
+    for (size_t idx = 1; idx <= KVS_MAX_SNAPSHOTS; ++idx)


Okay, so snapshot ID==0 is not considered snapshot? This might be fine, but consider documenting the approach to snapshot ID in score/kvs/docs/requirements/index.rst. It should be written somewhere that 0 is reserved for current storage, and snapshots are in <1; max snapshot> range.

Snapshot IDs and files index are two different things.
Snapshot IDs start from 0 to 2, while files index start from 0 to 3.
'kvs_0' is the current kvs while 'kvs_1', 'kvs_2' and 'kvs_3' are the snapshot files.

But Your requirements state that snapshot IDs start at 1?

arkjedrz · 2026-06-16T11:14:03Z

+                    const score::filesystem::Path dst_json{filename_prefix.Native() + "_" + to_string(new_snapshot_id) + ".json"};
+                    const score::filesystem::Path dst_hash{filename_prefix.Native() + "_" + to_string(new_snapshot_id) + ".hash"};
+
+                    const auto copy_json_res = filesystem->standard->CopyFile(src_json, dst_json);


This approach stores new snapshot based on snapshot 0 file. This can lead to surprising behavior if flush was not done prior to snapshot creation.
I'd consider storing current memory state as less surprising variant, but nonetheless this should be clearly stated.

arkjedrz · 2026-06-16T11:14:57Z

+    return count;
+}
+
+score::Result<std::size_t> Kvs::snapshot_create()


Please rework the function to return quickly on error. This will reduce if-else pyramids occurring in the code, with easier tracking on what's the method result.

arkjedrz · 2026-06-16T11:17:17Z

+        }
+        else
+        {
+            if (snapshot_count_res.value() == 0 || snapshot_id.id >= snapshot_max_count())


From previous methods I inferred that range is <1; max snapshot>, but this condition indicates that restoring for max snapshot is disallowed. Similar issue is observed in other methods.

Snapshot IDs start from 0 to 2. Therefore, it is not possible to restore the snapshot ID == 3 . Same check in snapshot_delete function

arkjedrz · 2026-06-16T11:24:04Z

-            else if (snapshot_count_res.value() < snapshot_id.id)
+            else if (!json_exists_res.value())
            {
+                logger->LogError() << "Snapshot JSON file does not exist: '" << json_path << "'";


Please check if it prints correctly, without added whitespaces surrouding path. If so, consider reworking those logs to logger->LogError() << "Snapshot JSON file does not exist:" << json_path;

arkjedrz · 2026-06-16T11:34:03Z

+    /* Check if files were created correctly */
+    EXPECT_TRUE(std::filesystem::exists(kvs_prefix + ".json"));
+    EXPECT_TRUE(std::filesystem::exists(kvs_prefix + ".hash"));
+    EXPECT_FALSE(std::filesystem::exists(filename_prefix + "_1.json"));


Add a comment that having a rotated snapshot was previous expected behavior, or remove check for snapshot ID 1.

arkjedrz · 2026-06-16T11:35:40Z

+    }
+
+
+    /* Crea snapshot non contigui e verifica che il conteggio sia corretto */


Please replace non-english comments.

Signed-off-by: Anna Paola <103565983+AnnaPaolaMusio@users.noreply.github.com>

arkjedrz

This might require comments from other people.

arkjedrz · 2026-07-06T14:13:15Z

-   The component shall maintain a configurable maximum number of snapshots.
+   The component shall create a snapshot in the first available slot when the snapshot_create function is explicitly called.
+   In the C++ implementation, slot 0 always holds the latest current the KVS (written on flush),
+   while slots 1 through 3 are available for explicitly created snapshots.


This is not true, when number of snapshots is configurable.

arkjedrz · 2026-07-06T14:15:06Z

-   :tags: inspected

-   The component shall maintain a configurable maximum number of snapshots.
+   The component shall create a snapshot in the first available slot when the snapshot_create function is explicitly called.


This is not true, when comp_req__kvs__snapshot_id define ID=1 as latest and ID=2,3,4... as older.
Maybe add clear explanation how creation and removal operations affect snapshot IDs available in the system.

arkjedrz · 2026-07-06T14:15:38Z


-   The component shall maintain a configurable maximum number of snapshots.
+   The component shall create a snapshot in the first available slot when the snapshot_create function is explicitly called.
+   In the C++ implementation, slot 0 always holds the latest current the KVS (written on flush),


Requirements should not discuss implementation details for one of the implementations.

arkjedrz · 2026-07-06T14:17:53Z

    else
    {
-        logger->LogError() << "Failed to create directory for KVS file '" << json_path << "'";
+        logger->LogError() << "Failed to create directory for KVS file : " << json_path;


Unnecessary whitespaces will occur.

arkjedrz · 2026-07-06T14:18:48Z

    size_t count = 0;
-    bool error = false;
-    for (size_t idx = 0; idx < KVS_MAX_SNAPSHOTS; ++idx)
+    for (size_t idx = 1; idx <= KVS_MAX_SNAPSHOTS; ++idx)


But Your requirements state that snapshot IDs start at 1?

arkjedrz · 2026-07-06T14:21:08Z

   :tags: inspected

-   The component shall rotate and delete the oldest snapshot when the maximum number is reached.
+   The component shall assign the ID 1 to the newest snapshot and shall increment the IDs of older snapshots accordingly.


It'd help to have a clear range of IDs available.

arkjedrz · 2026-07-06T14:24:44Z

   application to implement versioning, including upgrade and downgrade paths,
   as needed.

 .. comp_req:: Snapshot Creation


This requirement is no longer true with comp_req__kvs__snapshot_explicit_creation in effect. Current storage is something else than snapshots, right?

* This function creates a snapshot by copying the current KVS file (`*_0.json` and its corresponding `*_0.hash`) * into the first available snapshot file (file index : `*_1` .. `*_3`).

arkjedrz · 2026-07-06T14:26:02Z

+     * @brief Creates a new snapshot of the current kvs saved on the physical storage.
     *
-     * This function attempts to restore the key-value store to the state
+     * This function creates a snapshot by copying the current KVS file (`*_0.json` and its corresponding `*_0.hash`)


This is clear to me, but raising this topic as a point for discussion if snapshot_create should dump current file state or current memory state.

AnnaPaolaMusio added 4 commits May 25, 2026 15:36

feat: snapshots feature refactor for C++ part

5f89b3e

feat: added component requirement tests

ce1feab

feat: fixed component requirement tests c++

cb1bb1e

feat: fixed component requirement tests c++

429daff

AnnaPaolaMusio added 2 commits May 27, 2026 11:56

feat: fixed component requirement tests c++

90f647a

feat: fixed component requirement tests c++

c6ed728

AnnaPaolaMusio marked this pull request as ready for review May 27, 2026 11:43

AnnaPaolaMusio requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners May 27, 2026 11:43

umaucher reviewed May 27, 2026

View reviewed changes

Comment thread tests/test_cases/tests/test_cit_snapshots.py Outdated

AnnaPaolaMusio added 3 commits May 27, 2026 14:04

feat: fixed comment

eef40a2

fixed format

9c9d22a

delete snapshot_rotate comp.req.

b772d3a

arkjedrz requested changes Jun 16, 2026

View reviewed changes

AnnaPaolaMusio and others added 3 commits June 30, 2026 12:58

feat: fixed comment

874d1b4

Merge branch 'main' into snapshots-feature-refactor

db8675f

Signed-off-by: Anna Paola <103565983+AnnaPaolaMusio@users.noreply.github.com>

feat: fixed comment

0cf7b14

AnnaPaolaMusio requested a review from arkjedrz June 30, 2026 14:39

arkjedrz requested changes Jul 6, 2026

View reviewed changes

		}


		/* Crea snapshot non contigui e verifica che il conteggio sia corretto */

Uh oh!

Conversation

AnnaPaolaMusio commented May 26, 2026

Uh oh!

github-actions Bot commented May 26, 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 May 27, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 26, 2026 •

edited

Loading