kvs: adapted review points in library and tests

[sbachmann-qorix]

See
github.com/orgs/eclipse-score/discussions/853#discussioncomment-12856266 for details.

[github-actions]

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 49050bf1-fca9-4c7f-9dc4-6c7d8d83c2b6
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'googletest', the root module requires module version googletest@1.14.0, but got googletest@1.14.0.bcr.1 in the resolved dependency graph.
WARNING: For repository 'aspect_rules_lint', the root module requires module version aspect_rules_lint@1.0.3, but got aspect_rules_lint@1.4.2 in the resolved dependency graph.
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

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

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

Analyzing: target //:license-check (146 packages loaded, 923 targets configured)

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

Analyzing: target //:license-check (164 packages loaded, 2810 targets configured)

Analyzing: target //:license-check (164 packages loaded, 2810 targets configured)

Analyzing: target //:license-check (168 packages loaded, 4940 targets configured)

Analyzing: target //:license-check (169 packages loaded, 5060 targets configured)

Analyzing: target //:license-check (169 packages loaded, 5060 targets configured)

INFO: Analyzed target //:license-check (170 packages loaded, 6979 targets configured).
[9 / 13] Creating runfiles tree bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/score_dash_license_checker~/tool/formatters/dash_format_converter.runfiles [for tool]; 0s local ... (2 actions, 1 running)
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:31:21: Generating Dash formatted dependency file ... failed: missing input file '//:cargo_lock'
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:31:21: Generating Dash formatted dependency file ... failed: 1 input file(s) do not exist
Target //:license.check.license_check failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: /home/runner/work/inc_mw_per/inc_mw_per/BUILD:31:21 Generating Dash formatted dependency file ... failed: 1 input file(s) do not exist
INFO: Elapsed time: 185.664s, Critical Path: 0.33s
INFO: 12 processes: 2 disk cache hit, 10 internal.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

[joshualicht]

Findings regarding this PR:

• Function has_default_value
  Current Implementation:
  Return if the value is equal to its default
  Expected Behavior:
  Return if the key has a default value available
  Suggestion for improvement:
  Change Logic from has_default_value to the same as key_exists, just for the default values

• Function set_default_value
  Current Implementation:
  Overwrite the current value of the key with its default value
  Suggestion :
  There are currently differing understandings on how default values ​​should be set, so I would suggest to remove this feature until it is clearly defined.

• Function reset_key
  Current Implementation:
  This functionality is missing. It was reported under "Reset to Default Values"
  Suggestion:
  Implementation of a reset_key function with the logic used in the CPP Dev PoC (https://github.com/joshualicht/inc_mw_per/blob/cpp_poc/cpp/kvs.cpp)

• Data Separation between Applications:
  Current Implementation:
  dir parameter: Path to permanent storage
  Expected Behavior:
  Usage of the aligned naming convention:
  
  Suggestion :
  The naming scheme should be applied to the library. (Including refactoring of "dir" parameter to "process_name")
  Implementation example in the CPP Dev PoC is available (https://github.com/joshualicht/inc_mw_per/blob/cpp_poc/cpp/kvs.cpp)

• kvs_tool: _getkey
  Suggestion for improvement:
  This function can be simplified by the use of KVSValue.
  I will update the tool after this PR was merged.

General Findings:

• Function flush
  Current Implementation:
  Error Handling when witing a Hash File is not available: fs::write(filename_hash, hash.to_be_bytes()).ok();
  Suggestion for improvement:
  Check the return value of write with the "?"-operator, similar to writing JSON files.

• Function get_all_keys
  Current description:
  Get list of all keys
  Suggestion for improvement:
  Change description to point out, that only written keys are retrieved.
  e.g. : "Retrieves all keys stored in the key-value store. Important: It only retrieves the written keys, no default keys are returned."

• Snapshot functions: Rotate
  Current Implementation:
  Snapshot rotate is not thread safe
  Suggestion for improvement:
  Also lock Mutex for restoring snapshots to prevent race conditions

• Snapshot functions: Restore
  Current Implementation:
  Snapshot Restore is only partly thread safe
  Suggestion for improvement:
  Lock the whole function

• KVSBuilder construction
  Current Implementation:
  Method chaining is used to create a KVSBuilder. (e.g. let kvs = KvsBuilder::new(InstanceId::new(0)).dir(temp_path).build()?;)
  Suggestion for improvement:
  Create a constructor logic with all parameters to prevent chaining methods and refactor the chain methods to set methods.
  Background Information:
  The implementation of chaining methods in CPP will most likely not be MISRA compliant, so in order to have an equally working API, it needs to be replaced in RUST

[github-actions]

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