Re: async-mqtt5 is CONDITIONALLY ACCEPTED

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-10-31 15:52:16

On Thu, Oct 31, 2024, 11:40 PM Claudio DeSouza via Boost <
boost_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.
>

I have no previous ties with the author. The first time I heard of them was
when they asked me to be the review manager for their library.

I do not appreciate this kind of slander.

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
> >
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>