Re: async-mqtt5 is CONDITIONALLY ACCEPTED

From: Claudio DeSouza (cdesouza_at_[hidden])
Date: 2024-10-31 14:38:06

On Thu, Oct 31, 2024 at 2:20 PM Claudio DeSouza <cdesouza_at_[hidden]>
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_at_[hidden]> wrote:
>
>> 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.
>>
>> _______________________________________________
>> Unsubscribe & other changes:
>> http://lists.boost.org/mailman/listinfo.cgi/boost
>>
>