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.4.2) and connecting to it...
INFO: Invocation ID: 4f4c2791-12c5-4a87-a6e8-c90c5d541fb4
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
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: 
Analyzing: target //:license-check (4 packages loaded)
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)

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

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

Analyzing: target //:license-check (137 packages loaded, 2681 targets configured)

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

Analyzing: target //:license-check (150 packages loaded, 7955 targets configured)

Analyzing: target //:license-check (158 packages loaded, 8011 targets configured)

Analyzing: target //:license-check (158 packages loaded, 8011 targets configured)

Analyzing: target //:license-check (158 packages loaded, 8011 targets configured)

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

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

INFO: Analyzed target //:license-check (163 packages loaded, 10149 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
[16 / 17] Building license.check.license_check.jar (); 0s disk-cache, multiplex-worker
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: 25.852s, Critical Path: 2.56s
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:27:16Z

@@ -593,119 +593,141 @@ score::ResultBlank Kvs::flush()
            result = score::MakeUnexpected(ErrorCode::JsonGeneratorError);
        }
        else


Please use clang-format with config available in the repo for C++ files.

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.

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.

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshots feature refactor#292

Snapshots feature refactor#292
AnnaPaolaMusio wants to merge 9 commits into
eclipse-score:mainfrom
etas-contrib:snapshots-feature-refactor

AnnaPaolaMusio commented May 26, 2026

Uh oh!

github-actions Bot commented May 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 27, 2026

Uh oh!

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

arkjedrz Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		}


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

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!

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