Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Topic equalities: 'test/topic/' and 'test/topic' should be the same #46

Merged
merged 2 commits into from
Jul 16, 2013

Conversation

mcollina
Copy link
Collaborator

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?

@mcollina
Copy link
Collaborator Author

@davedoesdev here is the "dummy" implementation with a sequence of split, filter, join.
Do you think a regexp-based implementation can be better? Could you give it a go?

Should I merge this in and adress the "performance" part of this if the need arise?

@davedoesdev
Copy link
Contributor

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.

@davedoesdev
Copy link
Contributor

Advantage of normalizing the topic in mosca is each ascoltatore doesn't have to do it.
Disadvantage is each ascoltatore might be able to do it more efficiently

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.

@mcollina
Copy link
Collaborator Author

I think Ascoltatori should handle this, but Mosca too.
Ascoltatori needs it because it may be used outside Mosca.
Mosca needs it because it should 'fix' a packet's topic.

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.

@davedoesdev
Copy link
Contributor

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.
Mosca, however, has an external spec to conform with.

@mcollina
Copy link
Collaborator Author

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:

  1. remove any / at the beginning
  2. remove any / at the end
  3. remove any duplicate /, such as //

@davedoesdev
Copy link
Contributor

First cut:

> "//a///b/c//d///".replace(/(([^/])\/+$)|(^\/+([^/]))|(\/+(\/))/g, "$2$4$6")
'a/b/c/d'

Not very concise but the best I could come up with in limited time.

@mcollina
Copy link
Collaborator Author

Using your regexp it requires 6300ms to do 10 millions iterations, compared to 15000ms of the split/join approach! :)

@davedoesdev
Copy link
Contributor

Cool! One thing I thought of is just do a match first and then only replace if the topic matches.
The match should be quicker than a replace.
This would mean 'well-formed' topics should be quicker but 'malformed' ones will be slower.
Depends whether you want to bother with that. Speed increase would only show with mix of well-formed and malformed topics I guess.

@mcollina
Copy link
Collaborator Author

In fact, it requires around 20% more time to do the replacement.
However, matching and replacing requires 2x the time.

I think I will leave it as it is, with the regexp replacing every topic, it seems the better strategy.

@mcollina
Copy link
Collaborator Author

Do you think this is a 'patch' release or a minor one?

@davedoesdev
Copy link
Contributor

Unfortunately a patch I think - might change the behaviour if anyone's using a topic with duplicate separators for example.

mcollina added a commit that referenced this pull request Jul 16, 2013
Topic equalities: 'test/topic/' and 'test/topic' should be the same
@mcollina mcollina merged commit 471d6f8 into master Jul 16, 2013
@mcollina mcollina deleted the mosquitto-pattern-matching-tests branch July 16, 2013 10:44
mcollina added a commit that referenced this pull request Jul 17, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants