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

feat: include message id in emitted worker events #115

Merged

Conversation

DuckBoss
Copy link
Contributor

@DuckBoss DuckBoss commented May 3, 2023

The purpose of this update is to prepare for the implementation of a
message journal in yggdrasil by making small changes to workers.
This change adds the worker's message id to the emitted event data in addition
to the currently existing ipc worker event name and worker message data.

Current: ...EmitEvent(worker_event_name, worker_event_message)
Proposed: ...EmitEvent(worker_event_name, worker_event_message, worker_message_id)

Changes:

  • updated dbus interfaces to include new message id field.
  • updated "listen" yggctl action to include new message ids.
  • updated "WorkerEvent" dbus signals to include new message ids.

@DuckBoss DuckBoss marked this pull request as ready for review May 3, 2023 22:31
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

LGTM

@DuckBoss DuckBoss force-pushed the jajerome/add-message-id-to-worker branch from 0db1a61 to cd3f6a0 Compare May 5, 2023 18:54
@ahitacat
Copy link
Contributor

ahitacat commented May 8, 2023

What is the difference between this message_id and the id in the response_to field?

Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

This requires a change to the com.redhat.Yggdrasil1.Worker1 D-Bus interface definition too. That specification defines the fields that can be included in the Event signal, and adding a field such as this requires updating the D-Bus specification as well.

@subpop
Copy link
Collaborator

subpop commented May 9, 2023

What is the difference between this message_id and the id in the response_to field?

This is a signal emitted by the worker; it is not intended to be a means by which a worker can "respond" to messages. It's more like the worker saying "okay, I got the message 1234" rather than "here is message 5678 in reply to message 1234".

@DuckBoss DuckBoss force-pushed the jajerome/add-message-id-to-worker branch 4 times, most recently from 825bcce to a2b0c06 Compare May 9, 2023 18:55
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

This looks great. I just have one small nit-picky thing about function parameter order.

worker/worker.go Outdated
func (w *Worker) EmitEvent(event ipc.WorkerEventName, message string) error {
args := []interface{}{event}
// EmitEvent emits a WorkerEvent, worker message id, and an optional message.
func (w *Worker) EmitEvent(event ipc.WorkerEventName, message string, message_id string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we keep the order of arguments the same as the arguments of the D-Bus method? That reduces cognitive friction when translating between the D-Bus interface description and the implementations.

DuckBoss added 3 commits May 25, 2023 15:44
Signed-off-by: Jason Jerome <jajerome@redhat.com>
This patch also updates the "listen" yggctl action
and "WorkerEvent" dbus signals to include new message ids.

Signed-off-by: Jason Jerome <jajerome@redhat.com>
@DuckBoss DuckBoss force-pushed the jajerome/add-message-id-to-worker branch from 72aa12b to 034eb24 Compare May 25, 2023 19:45
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

I noticed a couple more small changes. One is a typo and one is a variable naming convention. Once those are fixed, I'll merge this. I tested it locally already, so it functions as expected.

worker/worker.go Outdated Show resolved Hide resolved
cmd/yggctl/actions.go Outdated Show resolved Hide resolved
Signed-off-by: Jason Jerome <jajerome@redhat.com>
@DuckBoss DuckBoss requested a review from subpop June 1, 2023 17:31
@subpop subpop merged commit e0911a4 into RedHatInsights:main Jun 2, 2023
@DuckBoss DuckBoss deleted the jajerome/add-message-id-to-worker branch June 5, 2023 19:23
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.

4 participants