Re: async-mqtt5 is CONDITIONALLY ACCEPTED

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-10-31 04:41:22

On Wed, Oct 30, 2024 at 9:01 AM Klemens Morgenstern
<klemensdavidmorgenstern_at_[hidden]> wrote:
>
> Hi all,
>
> thanks to everyone participating in the review! I really want to
> express my appreciation to the reviewers, especially those who
> invested multiple hours if not days to review this library: thank you
> Vinnie, Ruben, Mohammad and Marcelo.
>

I did not write a  summary on purpose,
because I didn't think criticizing a review was a good idea,
as it might discourage others from participating in reviews.

This is not new, summarizing the review & discussion has not always
happened in the review decision.

https://lists.boost.org/Archives/boost/2021/03/251099.php
https://lists.boost.org/Archives/boost/2022/06/253193.php
https://lists.boost.org/Archives/boost/2024/01/255717.php

However, having this review called a "miscarriage" because
I posted a conclusion after the (extended) review period ended,
showed my approach to be misguided.

I want to start by saying that "low-effort" in a review isn't a problem.
I rather see it as an issue that people seem to expect themselves
to invest hours into a review and if they can't, they don't write a review.
I think reviews that are done quickly are fine if they state that.
This is the reason that the outcome of a review is not determined by
majority vote.

However, a review should address the actual library and actual problems
or clearly state that it's about a personal preference.

Since I have received two reject reviews that I weighed on opposite
sides of the spectrum,
comparing them should be a useful exercise.

The review summary I should have written when announcing the result
can be found at the end.

# Review Analysis

## Low weighed
https://lists.boost.org/Archives/boost/2024/10/258159.php

> My primary reasoning is that so far almost all the review discussion I've
> seen is focusing solely on the Asio aspect which makes this a miss.

The review subject is the library at question, not the discussion around it.
What the review is actually referring to is essential for it's weighing.

> [...]  the actual meat and potatoes of the library [...] is supposed to be MQTT parsing and
> serialization and broker management.

This is an assertion without anything backing it up.
It might hold true for HTTP 1.1, yet many protocols have pretty easy
binary protocols,
but rather difficult state management. There is no technical basis
here, nor experience.
The review later says "I don't have a lot of experience with MQTT".

Such an assumption needs to be backed either by technical detail or by
experience. Neither is present.

Thirdly "broker management" gets mentioned. A broker is an MQTT server
and this is a client library.
Managing brokers in a client library requires IO.

> what I'd like to see in the
> future for libraries like these is a first-class interface _without_ I/O.

This is the core of this review: a personal preference of how
libraries like this have to be structured.
It's not a review of the library submitted, but a rejection of the
kind of library it is (an asio based client).

> I don't have a lot of experience with MQTT as a protocol but my operating
> assumption is that if TLS/SSL can be made without I/O, so can anything else.

Just because something can be done doesn't mean it's the right or even
a good idea.
It could also turn an I/O less MQTT library into an unusable callback hell.
The review states correctly that this is an assumption, and thus it
cannot get weighed as highly
as a discussion of an actual implementation.

In Summary, this review rejects async-mqtt5 not on the library's own merit,
but because it wants a fundamentally different approach to client libraries.
A rejection based on kind - especially when multiple libraries like
the submitted are already in boost -
would need more than a minority opinion.
Because of that the review gets weighed significantly lower,
because that requires more reviews with the same opinion to be submitted.

# Highly Weighed

https://lists.boost.org/Archives/boost/2024/10/258190.php

The other REJECT review got weighed significantly higher and I would
like to highlight a few things that made it valuable.

> It is useful for MQTT clients that are not resource constrained embedded systems.

First of all the review starts with a "it's useful for X" even though
it later rejects it.
This shows consideration of the general utility of the library and other users.

> I immediately ran into this lack of visibility when I used EMQX as the
> broker to test asnyc-mqtt5 against. As I was eventually able to deduce,
> EMQX doesn't like a blank client name (...). At least
> this sent me digging through the code to see how it works, adding
> "logging" so I could see when messages were being built, sent, received.
> I've taken a fairly thorough look at the code in my review.

This is very helpful: the review explains a problem and suggests a
solution (logging).

> Id really like to see a more composed/layered approach used.

Within the context of the review it's obvious why this is desired to
analyze the internal behaviour.
It's neither ideological nor fictional. In a follow-up email the
reviewer wrote this:

> A high level library that can do all this needs a tunable "QoS" in the
> general sense (not just the selection of one of 3 values for an MQTT
> parameter). A low level library can legitimately  claim its out of
> scope. This library has high level features but appears to lack some
> tunability (but that could be user ignorance on my part) and also access
> to low level features to allow the user to take responsibility for these
> issues. That needs addressing.

This also is very good review content, as it points out a problem but
doesn't prescribe a specific solution.
In a layered approach a user could tune this by accessing the layers,
or it could be by exposing this functionality in the
high level interface.
This is very helpful because it clearly states a problem to be solved.
The library author can use this as advise to improve his library
(whether accepted or rejected)
and the review manager can use those comments to form a decision or
formulate conditions.

I hope those two examples help future reviewers write impactful reviews.

# Review Summary

Overall there were two main worries shared by most reviews: lack of
transparency and compile times.
Additionally, a bulk publish and type erased connections were
recommended by three reviews.

Two REJECT recommendations:

https://lists.boost.org/Archives/boost/2024/10/258159.php
This review has little technical substance but promotes I/O less
protocol libraries.
It doesn't consider the MQTT protocol at any point,
but just assumes it is the correct approach without technical basis or
experience.

https://lists.boost.org/Archives/boost/2024/10/258190.php
The review criticizes the lack of fine-tuning and access to the
internal workings of the client.
It criticizes the lack of transparency, yet does consider the library useful.
In a follow-up email the reviewer states it's a "reject at this point",
meaning the reviewer might change it to an accept if the library moves
the right direction.

One ACCEPT recommendations:

https://lists.boost.org/Archives/boost/2024/10/258172.php
This review, with considerable asio experience, does take a detailed
look at the implementation, sadly without building the library.
The review criticizes property interface, but concludes the library
overall is solid enough to be in boost.

Three CONDITIONAL ACCEPT recommendations:

https://lists.boost.org/Archives/boost/2024/10/258189.php
The review points out some issues with the async_publish behaviour:
it should be able to publish more than one message at a time and
should not trigger write calls.
The two conditions for acceptance are:

1. Add logging.
2. Add a separately compiled connection using any_completion_handler.

NOTE: The reviewer did criticize the executor correctness prior to his
review and mentioned it in a recommendation.

https://lists.boost.org/Archives/boost/2024/10/258149.php
This review was approving of a high-level library based on the
reviewers prior work in the IoT world and his current work with asio.
The review shares the compile time concerns and recommends evaluation
of a type-erased stream.
It has three conditions for acceptance:

1. Debugging easier (e.g. logging)
2. Executor correctness
3. TLS/websocket shutdown

https://lists.boost.org/Archives/boost/2024/10/258164.php
The review likewise approves the high-level interface based on the
reviewer's prior experience with mqtt_cpp.
It likewise recommends evaluating a type-erased stream.

It has the following criteria for acceptance:

1. Error reporting
2. Break the hard dependency on Beast & TLS
3. Limits on internal queues
4. Sanitizer builds.

In Summary:

2 REJECT
1 ACCEPT
3 CONDITIONAL ACCEPT

The highly weighed reject review was declared "reject at this point"
by its author, i.e. not a categorical rejection.
That means that only one rejection is based on the fundamental idea of
what the library is, i.e. 5/6 reviews
generally agree that an asio based mqtt5 client library could or
should be accepted into boost.

Among the accepting reviews are three from authors of similar
libraries (Beast, Redis, MySql) and one the maintainer of Beast.
One has professional IoT experience, another direct experience with mqtt.

Therefore a CONDITIONAL ACCEPT is the correct conclusion of this review.

Regarding the conditions:

The logging/error issue was a condition of all three conditional
reviews and mentioned as a possible solution by one rejecting review.
The executor correctness is only a condition in one of the reviews,
but considered a bug by others during the discussion.
Since this is a violation of being "Fully Boost.Asio compliant" as
mentioned in the docs of async-mqtt5 it needs to be fixed.