Repository navigation
Replace floating-point seconds with integer milliseconds in timing config - #388
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
|
The build with new config feature flag Could you apply this simple fix, to get your branch build successfully with new config flag active? score/launch_manager/src/daemon/src/main.cpp: |
NicolasFussberger
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Changes look good to me.
As explained in another comments, I'd only revert the schema version increase, as the json schema is not changed here.
Please take a look at the PR checks, there are some small things to fix to make them pass.
6cb7ef1 to
662f5e5
Compare
662f5e5 to
ef9f1f4
Compare
ef9f1f4 to
31dcfb5
Compare
31dcfb5 to
1b86ad5
Compare
63402ea to
5fbc119
Compare
5fbc119 to
b0e30a6
Compare
|
hey @NicolasFussberger PR checks fixed, ready for review whenever you have the time. |
|
Documentation preview for this pull request is available at: |
NicolasFussberger
left a comment
There was a problem hiding this comment.
Thanks for updating the PR.
One minor point about the new polling_interval that was added recently, otherwise looks great to me
Per review feedback: FileState::polling_interval was the last timing field still using the old double-seconds flatbuffer pattern while every other field had already been migrated to a uint32 _ms field. - lm_flatcfg.fbs: polling_interval:double -> polling_interval_ms:uint32 = null, matching every other timing field's convention. - flatbuffer_type_converters.cpp: convertFileState() now reads polling_interval_ms directly via requireScalarValue(), same as every other _ms field. secondsToMs() is now completely unused and removed (it was only still called for this one field). - lifecycle_config.py: sec_to_ms() gains the three validation checks that used to live only in the now-removed C++ secondsToMs() (negative, uint32 overflow, sub-millisecond-rounds-to-zero), so bad configs are rejected at generation time for every _ms field, not just this one. The polling_interval -> polling_interval_ms conversion happens where every other _ms field is already converted, in gen_config()'s component-building loop; the user-facing JSON schema is intentionally left as polling_interval (seconds), matching how every other field keeps its user-facing name. - Test updates: removed the C++ SecondsToMsTest suite (function no longer exists) and the two negative/sub-millisecond convertFileState tests (that validation only happens in Python now); added the same five cases as sec_to_ms() unit tests in Python instead. The "missing polling_interval_ms" case changes from an assertion to a regular InvalidFormat error, consistent with every other required _ms field using requireScalarValue(). Regenerated full_config_test/expected_output/lm_config_gen.json.
c85b13e to
403c4c7
Compare
|
Hello @NicolasFussberger changes are applied please let me know if any other changes are requested |
|
thanks a lot @shegazyy. Would you also do the json schema changes as a follow up so that the user actually configured ms values instead of seconds? |
Rename the 8 user-facing timing fields in launch_manager.schema.json from seconds (number) to their _ms equivalents (integer), matching the flatbuffer schema migration done in eclipse-score#388: - reporting_cycle -> reporting_cycle_ms - polling_interval -> polling_interval_ms - delay_before_restart -> delay_before_restart_ms - ready_timeout -> ready_timeout_ms - shutdown_timeout -> shutdown_timeout_ms - transition_timeout -> transition_timeout_ms - evaluation_cycle -> evaluation_cycle_ms - max_timeout -> max_timeout_ms Remove the now-redundant sec_to_ms() conversion (and its validation) from lifecycle_config.py, since the JSON schema's type/minimum/maximum constraints already reject negative, non-integer and overflowing values. Update all example configs, integration test fixtures, the gen_lifecycle_config.py test helper, and documentation to use the new field names and millisecond values. This was previously postponed pending removal of legacy config support, which has since happened in eclipse-score#388. Closes eclipse-score#635
Rename the 8 user-facing timing fields in launch_manager.schema.json from seconds (number) to their _ms equivalents (integer), matching the flatbuffer schema migration done in #388: - reporting_cycle -> reporting_cycle_ms - polling_interval -> polling_interval_ms - delay_before_restart -> delay_before_restart_ms - ready_timeout -> ready_timeout_ms - shutdown_timeout -> shutdown_timeout_ms - transition_timeout -> transition_timeout_ms - evaluation_cycle -> evaluation_cycle_ms - max_timeout -> max_timeout_ms Remove the now-redundant sec_to_ms() conversion (and its validation) from lifecycle_config.py, since the JSON schema's type/minimum/maximum constraints already reject negative, non-integer and overflowing values. Update all example configs, integration test fixtures, the gen_lifecycle_config.py test helper, and documentation to use the new field names and millisecond values. This was previously postponed pending removal of legacy config support, which has since happened in #388. Closes #635
…nfig
All 8 timing fields in
new_lm_flatcfg.fbsare changed fromdouble(seconds) touint32(milliseconds), eliminating the need for thesecondsToMs()conversion helper and removing the floating-point representation from the binary format entirely.Changes:
new_lm_flatcfg.fbs: 8 timing fields changed fromdoubletouint32flatbuffer_config_loader.hpp:kExpectedSchemaVersionbumped 1 → 2flatbuffer_type_converters.hpp/.cpp: removedsecondsToMs()andkSecondsToMilliseconds; all 7 call sites simplified to direct assignmentflatbuffer_type_converters_UT.cpp: removed 5SecondsToMsTestcases; updated all FlatBuffer builder calls to pass millisecond integersflatbuffer_config_loader_UT.cpp: updated all builder calls likewiselifecycle_config.py(gen_config): addedsec_to_ms()conversion for all 8 timing fields so the generated JSON matches the new uint32 schemalaunch_manager.schema.json: updatedschema_versionenum from[1]to[2]"schema_version": 1→"schema_version": 2Closes #345