Skip to content
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.

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

refactor: add OperationHandlerActor #3032

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bravo555
Copy link
Contributor

Proposed changes

A preliminary OperationHandlerActor which still uses OperationHandler underneath, but receives command messages using the actor runtime.

This initial implementation shows the limitations of the entity store, which is now a part of CumulocityConverter, and other actors don't have any access to it. Because OperationHandlerActor needs some information about a registered entity (smartrest publish topic), I have added another MessageSource<(MqttMessage, EntityMetadata)> impl to the C8yMapperActor, but this obviously very hacky.

When moving more things out of CumulocityConverter into other actors, we'll need some organized way to access entity metadata (and entity store) from other actors. First thing that comes to mind is making an EntityStoreActor that manages the entity store and allows queries and updates to the store via incoming messages.

But we probably shouldn't allow all actors to modify the entity store freely, and we'll need to make sure what to do when entities are deleted or updated.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@Bravo555 Bravo555 changed the title add OperationHandlerActor refactor: add OperationHandlerActor Aug 1, 2024
@Bravo555 Bravo555 force-pushed the improve/operation-handler-actor branch from 7875d6a to be8879a Compare August 8, 2024 13:58
@Bravo555 Bravo555 force-pushed the improve/operation-handler-actor branch from be8879a to c345ca5 Compare August 14, 2024 14:39
Copy link
Contributor

github-actions bot commented Aug 14, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
491 0 2 491 100 1h21m28.159233999s

@rina23q
Copy link
Member

rina23q commented Aug 20, 2024

Honestly speaking, I'm pretty much lost in the design level, what the roles of OperationHandlerActor and converter for the operations. What is the benefit to split off the handler to one actor.

From the code, I see still converter is responsible for accepting c8y's operation MQTT messages (JSON over MQTT topic) and converting and publishing the thin-edge command format message (te/../cmd/<cmd>/<id>). If parse error occurs, converter publishes the SmartREST c8y's executing and failed messages.
This means that the possible source of SmartREST operation status update messages are distributed in converter and OperationHandlerActor.

I was actually expecting that one actor is addressing all the operation related things, including the conversion from c8y JSON over MQTT / SmartREST (only for custom operations) to thin-edge commands.

But the core issue is not this level. I believe we need a clear picture, how many components we will have, which component is connected to another component with directions (one-way or both ways). @albinsuresh made a proposal before, but it was a proposal. We don't have a design conclusion somewhere as reference of what you're doing (remark: "conclusion" not a stone, changeable during implementation, but overview of the refactoring). Without it, it's difficult to review because I'm unsure what I am requested to check...

A preliminary OperationHandlerActor which still uses OperationHandler
underneath, but receives command messages using the actor runtime.

This initial implementation shows the limitations of the entity store,
which is now a part of CumulocityConverter, and other actors don't have
any access to it. Because OperationHandlerActor needs some information
about a registered entity (smartrest publish topic), I have added
another `MessageSource<(MqttMessage, EntityMetadata)>` impl to the
`C8yMapperActor`, but this obviously very hacky.

When moving more things out of CumulocityConverter into other actors,
we'll need some organized way to access entity metadata (and entity
store) from other actors. First thing that comes to mind is making an
`EntityStoreActor` that manages the entity store and allows queries and
updates to the store via incoming messages.

But we probably shouldn't allow all actors to modify the entity store
freely, and we'll need to make sure what to do when entities are deleted
or updated.

Signed-off-by: Marcel Guzik <[email protected]>
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