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

Add MQTT source/destination #22

Merged
merged 9 commits into from
Aug 29, 2023
Merged

Add MQTT source/destination #22

merged 9 commits into from
Aug 29, 2023

Conversation

harpesichord
Copy link
Contributor

This needs some documentation to show what config options are needed, but it allows you to subscribe to an MQTT topic to receive messages or send messages to a topic.

@harpesichord harpesichord marked this pull request as ready for review August 8, 2023 01:05
Copy link
Contributor

@abraithwaite abraithwaite left a comment

Choose a reason for hiding this comment

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

First off, this looks amazing! It's very insightful to see how you're using it and thinking about the code.

I realized a little late in the review that I hadn't been paying close enough attention to the generic implementation type being types.Event.

This is one particular area that I think is important to distinguish the framework from the daemon (and perhaps break apart the library from the daemon).

You see, I imagined the framework to provide primitives where things like MQTT, Kafka, Redis, etc, would implement kawa.Source[[]byte], and consumers of these low-level APIs would do the serialization as another layer on top. Kawa the daemon, for example, would be one of those consumers of kawa the library.

All that being said, I think this is useful and we should merge it. But it definitely led to me confusing myself about how you were using msg.Value and json.Marshaling the rawLog.

This is definitely something that should be in the docs. Would especially love to hear your thoughts on it too!

internal/sources/mqtt/mqtt.go Outdated Show resolved Hide resolved
internal/sources/mqtt/mqtt.go Outdated Show resolved Hide resolved
internal/sources/mqtt/mqtt.go Outdated Show resolved Hide resolved
internal/destinations/mqtt/mqtt.go Outdated Show resolved Hide resolved
@ejcx
Copy link
Contributor

ejcx commented Aug 11, 2023

Any reason we shouldn't merge this @harpesichord ?

@harpesichord
Copy link
Contributor Author

harpesichord commented Aug 11, 2023 via email

@harpesichord
Copy link
Contributor Author

@abraithwaite Can you check this again, make sure I followed your new pattern correctly.

Copy link
Contributor

@abraithwaite abraithwaite left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! This is precisely what I had in mind :-)

It still feels a bit awkward having the extra indirection wrapping the event type just for kawad, but I think it will make the sources and destinations in the library much more broadly useful.

@harpesichord harpesichord merged commit b84d66f into main Aug 29, 2023
1 check passed
@harpesichord harpesichord deleted the harpe/mqtt branch August 29, 2023 00:42
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.

3 participants