Qos mqtt compatibility #238

LePailleurThibault · 2022-11-04T14:22:26Z

MQTT compatibility with QoS

Some MQTT brokers do not support qos=2.

Changing transport service to publish with qos=1.
Adding a cache in transport service to avoid sending redundant messages.

hound · 2022-11-04T14:22:31Z

python_transport/tests/test_arguments.py

@@ -153,6 +158,14 @@ def content_tests(settings, vcopy):
        vcopy["WM_GW_BUFFERING_MINIMAL_SINK_COST"]
        == settings.buffering_minimal_sink_cost
    )
+    assert (


Black would make changes.

python_transport/wirepas_gateway/transport_service.py

pgaucherand

Some minor changes with naming and comments to make but otherwise looks good.

python_transport/wirepas_gateway/protocol/cache_message.py

python_transport/wirepas_gateway/transport_service.py

python_transport/wirepas_gateway/utils/argument_tools.py

python_transport/wirepas_gateway/transport_service.py

python_transport/wirepas_gateway/protocol/mqtt_wrapper.py

python_transport/wirepas_gateway/protocol/cache_message.py

teemupiiroinenwirepas · 2022-12-13T17:42:47Z

python_transport/wirepas_gateway/protocol/cache_message.py

+        }
+        self.last_update_time = current_time
+
+    def is_in_cache(self, msg_id):


Does this take into account situation that all the messages are sent with same message id? I think that It has not been previously required that message id needs to distinct in some time range.

I thought that message id were supposed to be unique. Anyway, it is better to hash the payload and the topics and to compare in the cache if it is already present instead of taking the message id. It will therefore handle this kind of issue.

Is there some time stamp in the incoming message? In some cases you might send broadcast RemoteAPI message that is exactly the same as previous message after one or two minutes (or even faster in LL network) to be sure that the message goes through. In this case the hash would match also.

The uniqueness of request_id is specified here: https://github.com/wirepas/backend-apis/blob/master/gateway_to_backend/README.md#request-unique-identifier
So it is written that at least 48 bits must be unique.

Good. Is the some check in gateway for req id? If I remember correctly, at least earlier, it was possible to use any constant req id (e.g. always zero).

At the moment there is no check, it is just copied to the response. So if you use same id, you cannot match the response to an answer.
But with this proposed changed, this id wooul now be used as a way to detect duplicate messages. We could change it as a hash of full message, but will still filter twos messages with same id and same content sent very close in time.

In our case we are not so interested about the id as we cannot rely that the message reaches the target, so we need to resend it until we get response from the node.

The best is then to resend it with a new request_id as it is definitely not the same request

hound · 2023-01-02T14:12:48Z

python_transport/tests/test_cache_message.py

+    assert message_cache.add_msg(REQ_ID) is True
+
+    # Test if the cache has reset eventually
+    sleep(CACHE_TIME_WINDOW_S/2)


Black would make changes.
missing whitespace around arithmetic operator

hound · 2023-01-02T14:12:49Z

python_transport/wirepas_gateway/protocol/message_cache.py

+        self.cache_update_s = min(cache_update_s, cache_time_window_s)
+
+        # Start a thread that cleans periodically the cache.
+        self._clean_cache_thread = Thread(target=self._clean_cache_thread, daemon=True)


line too long (87 > 79 characters)

hound · 2023-01-02T14:12:49Z

python_transport/wirepas_gateway/protocol/message_cache.py

+    ):
+        """
+        Args:
+            cache_time_window_s: time in seconds after which a message is removed from the cache.


line too long (97 > 79 characters)

hound · 2023-01-02T14:12:49Z

python_transport/wirepas_gateway/protocol/message_cache.py

+    """
+    Class to cache all recent messages to avoid sending duplicates in MQTT.
+    """
+    def __init__(


Black would make changes.

hound · 2023-01-02T14:12:49Z

python_transport/wirepas_gateway/transport_service.py

+        # Add the message to the cache, if it can't be added,
+        # the function should not send back any message
+        if not self.message_cache.add_msg(request.req_id):
+            logging.warning(f"_on_otap_set_target_scratchpad_request_received: A message with id={request.req_id} is already present in the cache !")


line too long (149 > 79 characters)

hound · 2023-01-02T14:12:50Z

python_transport/wirepas_gateway/transport_service.py

@@ -559,6 +575,10 @@ def _on_set_config_cmd_received(self, client, userdata, message):
            logging.error(str(e))
            return

+        if not self.message_cache.add_msg(request.req_id):
+            logging.warning(f"_on_set_config_cmd_received: A message with id={request.req_id} is already present in the cache !")


line too long (129 > 79 characters)

hound · 2023-01-02T14:12:50Z

python_transport/wirepas_gateway/transport_service.py

@@ -536,6 +548,10 @@ def _on_get_gateway_info_cmd_received(self, client, userdata, message):
            logging.error(str(e))
            return

+        if not self.message_cache.add_msg(request.req_id):
+            logging.warning(f"_on_get_gateway_info_cmd_received: A message with id={request.req_id} is already present in the cache !")


line too long (135 > 79 characters)

hound · 2023-01-02T14:12:50Z

python_transport/wirepas_gateway/transport_service.py

+        # Add the message to the cache, if it can't be added,
+        # the function should not send back any response to the request
+        if not self.message_cache.add_msg(request.req_id):
+            logging.warning(f"_on_get_configs_cmd_received: A message with id={request.req_id} is already present in the cache !")


line too long (130 > 79 characters)

hound · 2023-01-02T14:12:50Z

python_transport/wirepas_gateway/transport_service.py

+        # Add the message to the cache, if it can't be added,
+        # the function should not send back any response to the request
+        if not self.message_cache.add_msg(request.req_id):
+            logging.warning(f"_on_send_data_cmd_received: A message with id={request.req_id} is already present in the cache !")


line too long (128 > 79 characters)

hound · 2023-01-02T14:12:50Z

python_transport/wirepas_gateway/utils/argument_tools.py

+
+        self.cache.add_argument(
+            "--cache_time_window_s",
+            default=os.environ.get("WM_CACHE_TIME_WINDOW_S", cleaning_time_window_s_default),


line too long (93 > 79 characters)

Some MQTT brokers do not support qos=2. Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages. Fixes on PR #238

Some MQTT brokers do not support qos=2. Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages.

Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages. Fixes on PR #238

Renaming class to MessageCache + normalizing variable names for better visibility. Cache is being cleaned with a thread and not with messages addition. QoS has been removed from mqtt option on publish and subscription, as only qos=1 should be used with the cache. Warnings have been added when redundancy is dectected at transport service level.

LePailleurThibault requested review from pgaucherand and GwendalRaoul November 4, 2022 14:22

hound bot reviewed Nov 4, 2022

View reviewed changes

pgaucherand requested changes Nov 9, 2022

View reviewed changes

GwendalRaoul requested changes Dec 12, 2022

View reviewed changes

teemupiiroinenwirepas reviewed Dec 13, 2022

View reviewed changes

LePailleurThibault force-pushed the qos_mqtt_compatibility branch from d1b0bb9 to a3d6971 Compare January 2, 2023 14:12

hound bot reviewed Jan 2, 2023

View reviewed changes

LePailleurThibault force-pushed the qos_mqtt_compatibility branch from a3d6971 to 6cff0bf Compare January 2, 2023 15:16

LePailleurThibault force-pushed the qos_mqtt_compatibility branch from 6cff0bf to 20f7828 Compare January 2, 2023 16:00

LePailleurThibault added 2 commits January 2, 2023 17:25

MQTT compatibility with QoS

4bc64b0

Some MQTT brokers do not support qos=2. Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages.

MQTT compatibility with QoS

7284e09

Some MQTT brokers do not support qos=2. Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages.

LePailleurThibault added a commit that referenced this pull request Jan 2, 2023

Some MQTT brokers do not support qos=2.

f1fd6a9

Changing transport service to publish with qos=1. Adding a cache in transport service to avoid sending redundant messages. Fixes on PR #238

LePailleurThibault force-pushed the qos_mqtt_compatibility branch from 20f7828 to f1fd6a9 Compare January 2, 2023 16:36

LePailleurThibault force-pushed the qos_mqtt_compatibility branch from f1fd6a9 to 307c3a2 Compare January 4, 2023 15:55

LePailleurThibault requested review from pgaucherand and GwendalRaoul January 4, 2023 15:59

Qos mqtt compatibility #238

Are you sure you want to change the base?

Qos mqtt compatibility #238

Uh oh!

Conversation

LePailleurThibault commented Nov 4, 2022

Uh oh!

hound bot Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgaucherand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teemupiiroinenwirepas Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LePailleurThibault Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

teemupiiroinenwirepas Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

GwendalRaoul Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

teemupiiroinenwirepas Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

GwendalRaoul Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

teemupiiroinenwirepas Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

GwendalRaoul Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

hound bot Jan 2, 2023

Choose a reason for hiding this comment

teemupiiroinenwirepas Dec 13, 2022 •

edited

Loading