Skip to content

update snapshot count function #216

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

Jump to bottom
Open
Ahmed-Elsaka-JC wants to merge 5 commits into
base: main
Choose a base branch
from
Open

update snapshot count function #216

Ahmed-Elsaka-JC wants to merge 5 commits into from

Conversation

@Ahmed-Elsaka-JC
Copy link
Contributor

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC commented Jan 15, 2026

  • update snapshot count function

Fixes #192

@Ahmed-Elsaka-JC
update snapshot count function
f87ae84 
- update snapshot count function

Fixes eclipse-score#192
@github-actions
Copy link

github-actions bot commented Jan 15, 2026 •
edited
Loading

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 (8.3.0) and connecting to it...
INFO: Invocation ID: 6dc0b031-1d00-481b-bc02-37d51d676954
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
DEBUG: Rule 'rust_qnx8_toolchain+' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-oEubHgeZDdT0svMmBKJx7c3/2TdSI/vfwRUyDn+TPGA="
DEBUG: Repository rust_qnx8_toolchain+ instantiated at:
  <builtin>: in <toplevel>
Repository rule http_archive defined at:
  /home/runner/.bazel/external/bazel_tools/tools/build_defs/repo/http.bzl:394:31: in <toplevel>
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'score_process', the root module requires module version [email protected], but got [email protected] 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: 
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)

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

Analyzing: target //:license-check (148 packages loaded, 1926 targets configured)

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

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

INFO: Analyzed target //:license-check (162 packages loaded, 9013 targets configured).
[11 / 14] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 62 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
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: 20.965s, Critical Path: 0.48s
INFO: 14 processes: 5 disk cache hit, 9 internal.
INFO: Build completed successfully, 14 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>]

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC force-pushed the afe_update_snapshot_count branch from 2bbfea7 to f87ae84 Compare January 15, 2026 13:24
@github-actions
Copy link

github-actions bot commented Jan 15, 2026

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

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC marked this pull request as ready for review January 19, 2026 10:37
@Ahmed-Elsaka-JC
update tests
b9743e1 
update tests

Fixes eclipse-score#192
@Ahmed-Elsaka-JC Ahmed-Elsaka-JC force-pushed the afe_update_snapshot_count branch from 9fdf6cf to b9743e1 Compare January 19, 2026 10:39
arkjedrz
arkjedrz suggested changes Jan 19, 2026
View reviewed changes
tests/python_test_cases/tests/test_cit_snapshots.py
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

Do not modify test logic.
Either expand list of xfails or (better) deal with other issue first #108

Copy link
Contributor Author

@Ahmed-Elsaka-JC Ahmed-Elsaka-JC Jan 19, 2026

Choose a reason for hiding this comment

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

@arkjedrz current logic is not correct for scenarios we have like snapshot_max_count = 0 or 3.

so may i ask why we are not allowed to change test logic ?

Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

Those tests expect snapshot_max_count to be parametrizable, and number of existing snapshots is compared to the number of max number of snapshots allowed.

tests/python_test_cases/tests/test_cit_snapshots.py
"snapshot_max_count": snapshot_max_count,
},
"count": 1,
"count": snapshot_max_count,
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

Do not change.

Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

This test is called TestSnapshotCountFirstFlush. It should always try to perform a single flush operation.
Results shall be as following:

  • snapshot_count must always == 1 for snapshot_max_count >= 1.
  • snapshot_count must always == 0 for snapshot_max_count == 0.

This cannot be done correctly without #108 fixed.

tests/python_test_cases/tests/test_cit_snapshots.py
"snapshot_max_count": snapshot_max_count,
},
"count": snapshot_max_count + 1,
"count": snapshot_max_count,
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

Do not change.

Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

This test is called TestSnapshotCountFull and the description states: "Checks that the snapshot count increases with each flush, up to the maximum allowed count.".
This test is to verify that snapshots pool saturates at the point specified by snapshot_max_count. It tries to flush one time more than there are snapshots allowed in the system - that's on purpose.
If You're seeing 2 snapshots were found when snapshot_max_count = 1 - it means that max value was not respected.

Again, this test is not possible to perform due to lack of #108

arkjedrz
arkjedrz suggested changes Jan 19, 2026
View reviewed changes
tests/python_test_cases/tests/test_cit_snapshots.py
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

Those tests expect snapshot_max_count to be parametrizable, and number of existing snapshots is compared to the number of max number of snapshots allowed.

tests/python_test_cases/tests/test_cit_snapshots.py
"snapshot_max_count": snapshot_max_count,
},
"count": 1,
"count": snapshot_max_count,
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

This test is called TestSnapshotCountFirstFlush. It should always try to perform a single flush operation.
Results shall be as following:

  • snapshot_count must always == 1 for snapshot_max_count >= 1.
  • snapshot_count must always == 0 for snapshot_max_count == 0.

This cannot be done correctly without #108 fixed.

tests/python_test_cases/tests/test_cit_snapshots.py
"snapshot_max_count": snapshot_max_count,
},
"count": snapshot_max_count + 1,
"count": snapshot_max_count,
Copy link
Contributor

@arkjedrz arkjedrz Jan 19, 2026

Choose a reason for hiding this comment

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

This test is called TestSnapshotCountFull and the description states: "Checks that the snapshot count increases with each flush, up to the maximum allowed count.".
This test is to verify that snapshots pool saturates at the point specified by snapshot_max_count. It tries to flush one time more than there are snapshots allowed in the system - that's on purpose.
If You're seeing 2 snapshots were found when snapshot_max_count = 1 - it means that max value was not respected.

Again, this test is not possible to perform due to lack of #108

@arkjedrz arkjedrz requested a review from PiotrKorkus January 19, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PiotrKorkus PiotrKorkus Awaiting requested review from PiotrKorkus

1 more reviewer

@arkjedrz arkjedrz arkjedrz requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

C++ KVS Does Not Update Snapshot Count on First Flush

2 participants

@Ahmed-Elsaka-JC @arkjedrz