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

[kleunen]

Using the subscription map it is possible to find connections which match a topic using wildcards.

[jonesmz]

What do you think of this?

https://www.boost.org/doc/libs/1_70_0/doc/html/property_tree.html#property_tree.intro

[jonesmz]

This is what I meant by having the exisiting multi-index code reference the subscription map that you're creating (or vice versa)
https://www.boost.org/doc/libs/1_70_0/libs/multi_index/doc/examples.html#example6

[jonesmz]

There's also https://www.boost.org/doc/libs/1_70_0/libs/graph/doc/table_of_contents.html

Which might be another way to solve this.

A tree structure is just another kind of graph, after all.

[kleunen]

The datastructure is not a graph, a graph has nodes (with an id), and edges (with connect node -> node). In our case, the edge has a label (the level name), so the connection is as follows node -> (level name) -> node.

[kleunen]

The foreign key from multi-index may actually be useful.

[kleunen]

Using a property_map is not a good idea. You have no control of what data structure is used to store the keys of the entries. I specifically choose a unorder_map to store all keys at all levels. This way you do not have different hash maps at different nodes (you have one big hash map to store all entries), and the hash map has O(1) lookup. Most likely the property map uses a map per level, which will definitly have lower performance. The performance of the mqtt broker is very dependent on the performance of the subscription map.

[kleunen]

You could use a multi-index with a hashed key, i think the multi-index memory usage is better than the unordered_map. But other associative container hash maps, such as google sparse map, may also have other performance benefits .. The unordered_map is a good place to start, it is versatile, readily available and good performance for many use cases.

[kleunen]

Regarding the foregin keys example from boost multi-index:
https://www.boost.org/doc/libs/1_70_0/libs/multi_index/doc/examples.html#example6

Please note that this example creates a temporary view which combines two tables. It is basicly an INNER JOIN. I am not sure if this is what you are looking for.

[kleunen]

Look here for a benchmark of hash maps:
https://tessil.github.io/2016/08/29/benchmark-hopscotch-map.html

Google dense hash map may be a good alternative for the unordered_map to improve performance. Or maybe this one:

https://probablydance.com/2018/05/28/a-new-fast-hash-table-in-response-to-googles-new-fast-hash-table/

[kleunen]

I added the stages qos to the session now, and the subscription map is now mapped to a shared session pointer, rather then a connection. Is this sometime that can be accepted now ?

The stored topic map is not used now, but I can add this in a different pull request

[jonesmz]

Is this sometime that can be accepted now ?

Apologizes for the delay in getting back to you.

I think the code is worth seriously pursuing.

@kleunen You'll need to rebase your code against the latest master branch to resolve the current merge conflict.

However, keep in mind that only @redboltz can merge things into this repository. So it'll ultimately be up to him.

@redboltz Would you be able to review this change?

[jonesmz]

BTW: @kleunen Have you considered how we might add support for things like session expiry, and publish message expiry, given the changes you're making here?

Of course, they wouldn't need to be part of this PR. It's just that my work in progress pull request modifies a lot of the same areas that your code does. So I figured I'd point it out.

[redboltz]

Sorry for the late reply.

@redboltz Would you be able to review this change?

Yes, but before I will review, I have some requests.

1. Please rebase the code from the current master.
2. Please check the coding rule and update the code.
3. Does the PR support wildcard subscribe/unsubscribe ? If it does, please add wildcard tests.

[jonesmz]

@kleunen

I know it's been a long road since you first started working on this, but would you be willing to update your PR based on @redboltz request?

If not, would you mind if I rebased your work?

[kleunen]

I would like to update, but I have not been able to find the time to finish the work.

You can go ahead and rebase the work, if you keep me updated about the work you have done.

[jonesmz]

Ok.

Time allowing for my schedule, I'll probably take a stab at this early next week.

[kleunen]

@jonesmz Have you made any progress on this ?

[jonesmz]

@kleunen

I apologize: I haven't. At the time I intended to spend 2 days that week to work on this, but changing customer circumstances required I spent that time on other tasks.

Going forward, I'm changing jobs, and won't be using mqtt_cpp for work anymore. So any contributions I make in the future will be either for fun, or to wrap up efforts that I already started.

[redboltz]

@kleunen , I just added the PR #676 . It is for rebased version of #558 .
I already added and applied mqtt_cpp coding rules to some header files you add at #558.

Now, I started updating test_broker.hpp.

I come up with some questions.

1. Why connection_session_t has qos member ?

https://github.com/kleunen/mqtt_cpp/blob/master/test/test_broker.hpp#L1137
I think that QoS is not related to wildcard.

sub_con has qos_value member.
https://github.com/kleunen/mqtt_cpp/blob/master/test/test_broker.hpp#L1164

2. Responsibility of session_state, connection_session_t, subscription_map_t, and sub_con ?

session_state has con.
https://github.com/kleunen/mqtt_cpp/blob/master/test/test_broker.hpp#L1089

sub_con has con too.
https://github.com/kleunen/mqtt_cpp/blob/master/test/test_broker.hpp#L1163

I guess that there are some redundant member.
If I could have some diagram (like class diagram) that describes responsibility of those classes, it could be a great help to understand.

3. Why shared_ptr of session_state is required ?

I guess that in order to understand the reason, I need to understand the answer of the question 2 first.

[kleunen]

1. The qos in connection_session_t is not needed, you can get the qos from sub_con. connection_session_t is only needed to also store qos, but if qos is removed it is no longer needed.
2. subscription_map_t is the subscription map type, a relationship from subscription to a session. session_state and sub_con, i can't really comment on, they were there already.
3. session_state is stored both in the subscription map (subscription -> session), as well as in a list of all active sessions (mi_active_sessions ).

[kleunen]

test_broker still requires some updates, after importing subscription_map and respective tests, right ?

[redboltz]

test_broker still requires some updates, after importing subscription_map and respective tests, right ?

Yes, #687 is not pure test for subscription_map.
I have higher priority to proof how to introduce subscription_map into broker rather than subscription_map functionality tests.
I guess that the tests for subscription_map has already been done by you. If you add the tests, it would be very helpful.

I think that subscription_map.cpp is not good name for broker oriented tests. I will remove the filename to something like subscription_map_broker.cpp. Then you can add pure subscription_map's functionality tests as subscription_map.cpp.

The next step is proofing how to introduce retained_topic_map to the broker.

When I done both, I will update test_broker.hpp to introduce subscription_map and retained_topic_map.

[codecov]

Codecov Report

Merging #558 into master will increase coverage by 0.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
+ Coverage   82.23%   82.75%   +0.51%     
==========================================
  Files          47       45       -2     
  Lines        7100     6000    -1100     
==========================================
- Hits         5839     4965     -874     
+ Misses       1261     1035     -226     

[redboltz]

I close the PR to avoid confusion.

The base tools for wildcard subscription has already been merged into the master.
I will integrate trie based wildcard matching to the test_broker.hpp later. It would be a new PR.