Re: [boost] async-mqtt5 is CONDITIONALLY ACCEPTED

On Thu, Oct 31, 2024 at 2:20 PM Claudio DeSouza wrote:

Maybe you should have recused yourself from this review, considering you have previous ties to the author that may have made you quite biased against reject reviews, like calling them low effort without having any idea how much effort was put into them.

Robert Ramey did recuse himself from reviewing the other library precisely because he knew the author and he didn’t want to conduct a questionable review. I’m not saying that knowing an author is a problem, but it does look like this library was already accepted before the review started.

Claudio

On Thu, 31 Oct 2024 at 04:41, Klemens Morgenstern via Boost < boost@lists.boost.org> wrote:

...

On Wed, Oct 30, 2024 at 9:01 AM Klemens Morgenstern 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.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost