rumqtt icon indicating copy to clipboard operation
rumqtt copied to clipboard

Reduce duplicated code + improve structure

Open jspngh opened this issue 3 months ago • 4 comments

Hi,

First of all, thanks for creating and maintaining this repository. I do see some things that I think can be improved, and would like to contribute here. These are not small changes though, so I'll lead with the main question I have: I read in other issues that you are working in an internal repo and will later add changes here. That makes it a bit hard for external contributors to make larger changes. Are you interested and willing to accept large changes, and if so, how would that process look like?

I'll now detail a bit more what changes I would propose. The main issue I'd like to tackle is the duplicated code. There's a lot of copy-paste between the MQTT 3.1.1 and MQTT 5.0 implementation for the client. I don't think this is necessary, and makes maintenance a lot harder. Additionally, the protocol implementation could be shared between rumqttc and rumqttd.

I've already worked on an implementation in my fork, but you'll see that the diff is large. What I've done:

  • extract the protocol (the two mqttbytes folders for rumqttc) into a new rumqtt-bytes crate This implements version 4 and 5 of the protocol in a similar way as it's done in rumqttd/src/protocol. This crate then used in rumqttc and the mqttbytes folders are removed. I haven't used it in rumqttd yet, but that should be pretty straightforward.
  • unified the client, eventloop, state, ... implementations in rumqttc To come to a single implementation that works for MQTT 3.1.1 and 5.0.
  • restructured some of the code for better readability Created new modules for the Transport and MqttOptions types.
  • moved to a common Properties structure that can be used for all packets Instead of having packet specific properties (ConnectProperties, SubscribeProperties, PublishProperties, ...) I think it makes sense to have a common Property type and store the properties as a Vec<Property>. That does incur a heap allocation, but since every String or Vec property already allocates, I don't think this has a big impact. There are many advantages though:
    • the packet becomes smaller, since the properties are now on the heap instead of in the packet itself
    • you only need space for the properties that exist, not all possible properties for a packet
    • common implementation of property handling instead of having to duplicate it for each packet type
  • reworked the eventloop implementation to be easier to understand and maintain

These changes touch just about everything in rumqttc, so the diff is very large, as I already said. I'm interested to hear what you think about it, and what changes you've already made in your internal repo.

jspngh avatar Sep 14 '25 07:09 jspngh

@jspngh we are currently working on the new architecture for rumqttc. we shall publish new architecture ASAP.

giridher-art avatar Sep 19 '25 10:09 giridher-art

Hey @giridher-art,

What's the current status on your end? I'd like to contribute improvements here, but it seems you don't currently have the bandwidth for the open-source maintenance of this library.

Could you tell me if either:

  • the situation will change in the near future and you will have more bandwidth
  • you are willing to accept outside help for the maintenance

If neither is true (which is totally fine, it's just a matter of setting the right expectations) I know that I don't have to wait and need to look somewhere else.

jspngh avatar Nov 14 '25 16:11 jspngh

hey @jspngh we are working on new architecture of rumqttc as well as new mqtt broker . we shall release all the further details ASAP and also , we appreciate any outside contributions as well.

giridher-art avatar Nov 14 '25 16:11 giridher-art

Thanks for the update, I'll stay patient then.

jspngh avatar Nov 14 '25 21:11 jspngh