Skip to content

Comments

Maciejbacalext fixed deadline monitor#8

Closed
maciejbacalext wants to merge 5 commits intomainfrom
maciejbacalext_fixed_deadline_monitor
Closed

Maciejbacalext fixed deadline monitor#8
maciejbacalext wants to merge 5 commits intomainfrom
maciejbacalext_fixed_deadline_monitor

Conversation

@maciejbacalext
Copy link

No description provided.

TomaUngureanu and others added 5 commits November 3, 2025 09:59
Co-authors: Maciej Bacal <maciej.bacal.ext@qorix.ai>, Toma Ungureanu <TomaU_Ext.qorix.ai>
Signed-off-by: Toma Ungureanu <TomaU_Ext@qorix.ai>
@github-actions
Copy link

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]
2025/12/22 22:39:30 Downloading https://releases.bazel.build/8.5.0/release/bazel-8.5.0-linux-x86_64...
WARNING: Invoking Bazel in batch mode since it is not invoked from within a workspace (below a directory having a MODULE.bazel file).
Extracting Bazel installation...
OpenJDK 64-Bit Server VM warning: Options -Xverify:none and -noverify were deprecated in JDK 13 and will likely be removed in a future release.
ERROR: The 'run' command is only supported from within a workspace (below a directory having a MODULE.bazel file).
See documentation at https://bazel.build/concepts/build-ref#workspace

Comment on lines +18 to +33
enum class hm_Status : int
{
Running,
Disabled,
Failed,
};

enum class hm_Error : int
{
NoError,
BadParameter,
DoesNotExist,
NotAllowed,
OutOfMemory,
Generic,
};

Choose a reason for hiding this comment

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

Suggested change
enum class hm_Status : int
{
Running,
Disabled,
Failed,
};
enum class hm_Error : int
{
NoError,
BadParameter,
DoesNotExist,
NotAllowed,
OutOfMemory,
Generic,
};
enum class hm_Status : uint32_t
{
Running,
Disabled,
Failed,
};
enum class hm_Error : uint32_t
{
NoError,
BadParameter,
DoesNotExist,
NotAllowed,
OutOfMemory,
Generic,
};

For consistency

namespace hm
{

struct Deadline

Choose a reason for hiding this comment

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

Suggested change
struct Deadline
class Deadline

hm_Deadline *ptr;
};

struct DeadlineMonitor

Choose a reason for hiding this comment

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

Suggested change
struct DeadlineMonitor
class DeadlineMonitor

@@ -0,0 +1,178 @@
// Copyright (c) 2025 Contributors to the Eclipse Foundation
Copy link

@TomaUngureanu TomaUngureanu Dec 23, 2025

Choose a reason for hiding this comment

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

Please stick to one class per file. I expect something like:

deadline_monitor_ffi.hpp
deadline_monitor.hpp
deadline_monitor_builder.hpp
deadline.hpp

}
}

pub fn status(&self) -> Status {

Choose a reason for hiding this comment

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

Suggested change
pub fn status(&self) -> Status {
pub(crate) fn status(&self) -> Status {

// SPDX-License-Identifier: Apache-2.0
//
use crate::common::{Error, Status};
use crate::common::{Error, Status, Tag, DurationRange};
Copy link

@TomaUngureanu TomaUngureanu Dec 23, 2025

Choose a reason for hiding this comment

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

I would expect that in a final version, we should define builder, monitor, deadline and specific tests in separate files

Copy link

@TomaUngureanu TomaUngureanu left a comment

Choose a reason for hiding this comment

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

I would kindly ask you to rebase upon the Rust infra activation PR for the final folder structure: eclipse-score#24

Please let me know if a deadline module is feasible (monitor.rs, builder.rs, deadline.rs etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants