Added basic part of new wildcard mechanism. #676

redboltz · 2020-10-12T12:43:51Z

No description provided.

codecov · 2020-10-12T12:56:27Z

Codecov Report

Merging #676 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #676   +/-   ##
=======================================
  Coverage   82.70%   82.70%           
=======================================
  Files          46       46           
  Lines        7082     7082           
=======================================
  Hits         5857     5857           
  Misses       1225     1225

jonesmz · 2020-10-13T18:57:35Z

test/path_tokenizer.hpp

+     */
+    for (size_t idx = topic.find_first_of("+#");
+         MQTT_NS::string_view::npos != idx;
+         idx = topic.find_first_of("+#", idx+1)) {


I strongly recommend using the new version of this function from #672

It includes several fixes for bugs that I found when I added new tests.

Sorry ? Do you mean the current (merged) wildcard support has bugs and you fixed it in #672 ?
I thought that #672 is for shared subscriptions, not wildcard. But it seems that #672 contains wildcard bug fix.

I have no plan to merge #672. So it seems that I need to separate wildcard fixed part and create the new PR to fix wildcard support. And then merge the new PR.

I don't now much about path_tokenizer.hpp. Hopefully @kleunen , or I reflect the new PR to path_tokenizer.hpp someday before merging #676

I added #681 . It is from #672 . I will merge it after all tests passed. I will do soon.

Then reflect the fix to path_tokenizer.hpp. It is lower priority for me.

#681 merged.

Dear redboltz. Thank you for your effort in merging. I will have a look at the questions you have.

@kleunen , thank you for the reply.

I think that there are two topics to do.

The first one is simple. It relates to this thread. It seems that @jonesmz refined and fixed some bugs on his version of wildcard mechanism. And I believe that I already merged the fix by #681. I guess that the same problem is in path_tokenizer.hpp. I expect that you would fix it. In order to do that, you can write a PR and its target branch is new_wc.

The second one is bigger. I updated your code on new_wc. But test_broker.hpp is not yet updated because I need to understand the your design concept. I guess that you could do better.
Thanks to @jonesmz comment, I understand that the current wildcard approach is based on linear search algorithm. But your approach is based on trie algorithm. So your algorithm is more efficient. I want to replace the algorithm.

NOTE: I'm implementing other MQTT v5 feature now. I might force push to new_wc. If it avoids your work, please let me know.

redboltz. I looked at fix #681, it is not completely clear to me I understand what you fixed. I think if I read the code, you added a compare_filter and subscriptions get merged when two subscriptions match: one has wildcard, but other does not, but they both resolve to the same topic, is that correct ?

Or is it, the these rules from specification were not followed correctly ?
https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901247

I also see this comment in the code which is important for the matching:
// TODO: The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names beginning with a $ character

Regarding your second question, yes, the datastructure is like a trie or a multi-dimensional (or multi-level) associative array. The subscription is split into the respective components of the path ( l1/l2/l3 is split into l1, l2, l3) and the value is stored at the respective key.
Key for dimension or level 1 is: l1
Key for dimension 2 is: l2
Key for dimension 3 is: l3
So: map[l1][l2][l3] = value

Only thing is the lookup operation of the map, allows for matching wildcards, so:
map[l1][l2][l3] returns -> value

But i case the key is a wildcard:
map[l1][l2][*] return -> wildcard_value

Several lookups will return this value:
map[l1][l2][something] returns -> wildcard_value
map[l1][l2][l3][l4][l5] returns -> wildcard_value

map[something][l2][l3] -> Does not return wildcard_value

redboltz. I looked at fix #681, it is not completely clear to me I understand what you fixed. I think if I read the code, you added a compare_filter and subscriptions get merged when two subscriptions match: one has wildcard, but other does not, but they both resolve to the same topic, is that correct ?

I'm not sure about the fix. I guess that original temporary wildcard support contains some bugs and they are fixed by #681.

compare_topic_filter() is a comparison function for linear search. Please see calling locations.

compare_topic_filter() is called when publish packet delivering and when retained publish delivering on subscribe process.
The first parameter topic_filter is subscription's topic filter. It might contain wildcard. The second parameter topic_name is from publish packet. If it is matched, then published packet is delivered.

I think that compare_topic_filter() is replaced with trie version algorithm that you would modify test_broker.hpp in the future.

Or is it, the these rules from specification were not followed correctly ?
https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901247

I don't understand what do you mean at the above.

I also see this comment in the code which is important for the matching:
// TODO: The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names beginning with a $ character

So far, '$' (preserved for broker) prefix is not cared. It should be cared the new (trie) version.

I'm not sure I answered all of your question. Please let me know if you have further questions.

Regarding your second question, yes, the datastructure is like a trie or a multi-dimensional (or multi-level) associative array. The subscription is split into the respective components of the path ( l1/l2/l3 is split into l1, l2, l3) and the value is stored at the respective key.
Key for dimension or level 1 is: l1
Key for dimension 2 is: l2
Key for dimension 3 is: l3
So: map[l1][l2][l3] = value

Only thing is the lookup operation of the map, allows for matching wildcards, so:
map[l1][l2][l3] returns -> value

But i case the key is a wildcard:
map[l1][l2][*] return -> wildcard_value

Several lookups will return this value:
map[l1][l2][something] returns -> wildcard_value
map[l1][l2][l3][l4][l5] returns -> wildcard_value

map[something][l2][l3] -> Does not return wildcard_value

Does this work with a/+/b case ?

kleunen

I used @jonesmz his code to update the checking of the subscription string, in PR #684

kleunen · 2020-10-14T06:38:38Z

test/path_tokenizer.hpp

+     */
+    for (size_t idx = topic.find_first_of("+#");
+         MQTT_NS::string_view::npos != idx;
+         idx = topic.find_first_of("+#", idx+1)) {


redboltz. I looked at fix #681, it is not completely clear to me I understand what you fixed. I think if I read the code, you added a compare_filter and subscriptions get merged when two subscriptions match: one has wildcard, but other does not, but they both resolve to the same topic, is that correct ?

Or is it, the these rules from specification were not followed correctly ?
https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901247

I also see this comment in the code which is important for the matching:
// TODO: The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names beginning with a $ character

redboltz mentioned this pull request Oct 12, 2020

Using subscription map to allow for wildcard processing in mqtt test broker #558

Closed

redboltz force-pushed the new_wc branch 2 times, most recently from 987ffa5 to e1894f8 Compare October 13, 2020 13:47

jonesmz reviewed Oct 13, 2020

View reviewed changes

Added basic part of new wildcard mechanism.

d6d4fa7

redboltz force-pushed the new_wc branch from e1894f8 to d6d4fa7 Compare October 14, 2020 06:35

kleunen reviewed Oct 14, 2020

View reviewed changes

redboltz mentioned this pull request Oct 14, 2020

Subscription map updated checking of subscription path #684

Closed

redboltz closed this Oct 15, 2020

redboltz mentioned this pull request Oct 15, 2020

implement trie based wildcard support #686

Closed

Added basic part of new wildcard mechanism. #676

Added basic part of new wildcard mechanism. #676

Uh oh!

Conversation

redboltz commented Oct 12, 2020

Uh oh!

codecov bot commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleunen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 12, 2020 •

edited

Loading