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

Router support for MQTT messages #690

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

alexgtn
Copy link

@alexgtn alexgtn commented Sep 7, 2017

Related to vert-x3/vertx-mqtt#55

This PR is just to inform about changes to support MQTT messages.

As it was mentioned in the issue, the router needs a separate package (with the new changes).
Other option is to copy the relevant router code (with the new changes) to vertx-mqtt.
Thus, leaving vertx-web code unchanged.

Waiting for a decision on this from: @vietj @pmlopes @ppatierno

Test file: https://gist.github.com/AGutan/6f171403a778f33771f5307e03a3aabf

@vietj vietj added the to review label Sep 7, 2017
@alexgtn
Copy link
Author

alexgtn commented Sep 8, 2017

Update:

I did quite a lot of bug fixes. The MQTT router is working.

The MQTT router is based on the web router but is a tiny version of it, because it does't deal with headers, locales and other heavy stuff the web router handles when dealing with httprequests.

I'll make the PR with the updates to vertx-mqtt.

@vietj
Copy link
Contributor

vietj commented Sep 8, 2017

@AGutan I really would like to avoid duplicate much code, did you duplicate much ?

@alexgtn
Copy link
Author

alexgtn commented Sep 8, 2017

@vietj I'll make a PR and you'll see. Anyway avoiding duplication in this case is inevitable I think. Otherwise if you try to avoid duplication and mix mqtt and web interfaces together its even worse.

@pmlopes
Copy link
Member

pmlopes commented Sep 9, 2017

I don't see the link between web and mqtt I think this isn't the place for this code. I guess the best would be having a router common project with the base interfaces and common utility methods and having both web and mqtt extend it to the respective protocols

@vietj
Copy link
Contributor

vietj commented Sep 9, 2017

that's my opinion as well @pmlopes specially that we have a new implementatino coming ? perhaps this should be discussed on vertx-dev ?

@alexgtn
Copy link
Author

alexgtn commented Sep 9, 2017

@vietj the new router implementation is available somewhere? or it's going to be published with 3.5.0?

@pmlopes
Copy link
Member

pmlopes commented Sep 9, 2017

@AGutan no, the whole project is still in planning phase. Our GSoC student proposed to change the internals of the router from a list to a trie or ctrie in order to do faster iterations and lookups, provide a way to determine groups of handlers per path so http head metadata can be easily generated, reduce the amount of regular expression we currently do and finally have a properly working way to support sub routers.

There is no code yet but I believe that your mqtt router would also benefit of this project.

Now if we extract this to a sub module then it could be used by web, mqtt and if we want to go big why not other things to?

See: #678

@alexgtn
Copy link
Author

alexgtn commented Sep 9, 2017

@pmlopes yep then sub module sounds good.

ok then, looking forward for the new implementation.
Imho the base interfaces should be well designed to accept all kinds of objects httprequest, mqttmessage etc. so the new type implementations could be aggregated and avoiding as much duplication.

When it will be available I'll check the new implementation and aggregate the mqtt functionality.

@vietj
Copy link
Contributor

vietj commented Sep 9, 2017

I don't mind duplicating the API (this is debatable) because both probably don't share the same concerns, however at least the implementation (and bugs!!!) could be shared.

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Sep 25, 2017

I've talked with @ppatierno about joining MQTT and HTTP router in a single routing algorithm. I think It can be achieved by creating a unique Router implementation and then two different implementations of Route class (that contains the match() function and can handle MQTT wildcards): MQTTRoute and a HTTPRoute. Give a look at my blog post for more info: https://slinkydeveloper.github.io/articles/Routing-Tree-vs-SkipList/
I'm starting now the work on new Router so if you are interested I can also create the MQTTRoute implementation

@alexgtn
Copy link
Author

alexgtn commented Sep 25, 2017

@slinkydeveloper ok, sounds good.

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

Successfully merging this pull request may close these issues.

4 participants