-
-
Notifications
You must be signed in to change notification settings - Fork 506
Topic equalities: 'test/topic/' and 'test/topic' should be the same #46
Conversation
@davedoesdev here is the "dummy" implementation with a sequence of Should I merge this in and adress the "performance" part of this if the need arise? |
Also need to consider duplicate topic separators: http://mqtt.org/wiki/doku.php/topic_format This is tricky in that it seems a shame to incur the performance penalty for 'malformed' topics. |
Advantage of normalizing the topic in mosca is each ascoltatore doesn't have to do it. Reluctantly I'd say from an architectural point of view its probably mosca's job to clean up the topic before passing it on. What do you think @mcollina ? Another way to ask the question is what should ascoltatori specify as the topic format - i.e. what should it do about trailing and duplicate separators? Once we know that then we know the answer to this issue. |
I think Ascoltatori should handle this, but Mosca too. The current 'fix' solves it, but it is highly inefficient, as it split and join a string. One solution might be using an lru cache for caching the fixes, but it's just cpu vs memory. |
Ascoltatori could specify that it doesn't do anything special about duplicate or trailing separators. That seems the most efficient thing for it to do. |
Seems fine: it should be handled here, and Ascoltatori should state it in the readme. How could we do it here in a fast way? We need a single regexp that:
|
First cut:
Not very concise but the best I could come up with in limited time. |
Using your regexp it requires 6300ms to do 10 millions iterations, compared to 15000ms of the split/join approach! :) |
Cool! One thing I thought of is just do a match first and then only replace if the topic matches. |
In fact, it requires around 20% more time to do the replacement. I think I will leave it as it is, with the regexp replacing every topic, it seems the better strategy. |
Do you think this is a 'patch' release or a minor one? |
Unfortunately a patch I think - might change the behaviour if anyone's using a topic with duplicate separators for example. |
Topic equalities: 'test/topic/' and 'test/topic' should be the same
At the moment they are not. It is also valid for '/test/topic' and 'test/topic'.
I am not so sure that we should fix this here or in Ascoltatori.
The main reason for handling it in Mosca is to 'fix' the packet, so even in the persistance phase should not be considered again. The other way is I think we should also fix it in Ascoltatori.
@davedoesdev, what do you think?