Re: aedis review starts today

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2023-01-21 08:13:43

On Fri, 20 Jan 2023 at 22:24, Christian Mazakas via Boost
<boost_at_[hidden]> wrote:
>
> Hey everyone,
>
> This is my review of the proposed Aedis library. Please take all
> opinions with a grain of salt.
>
> > What is your evaluation of the design?
>
> It's by and large idiomatic Asio down to a T. If one knows Asio well, this
> library will integrate perfectly with all their existing code and idioms.
>
> Though I find myself perplexed by the documentation suggesting the necessity for
> having reconnections while then delegating that responsibility to me, the
> user.

To implement automatic reconnection the library would have to make
many decisions on behalf of the user

* How much should I wait before trying to reconnect?
* Should backoff be implemented?
* Reconnect to the same instance or a replica?
* Resolve names over a Redis sentinel?
* Where should I log reconnect errors?
* When should I stop trying to reconnect?

<snip>

> The design seems to preclude allowing users to handle I/O on their
> terms which I dislike.
>
> The lowest level interface is `resp3::read()` which is dependent
> upon Asio concepts.

Would you like to have the parser public in the API? What "handle I/O
on their terms" means exactly.

<snip>

> > What is your evaluation of the documentation?
>
> Stylistically it sets a new standard for how Boost libraries should
> look and feel.
>
> The examples are clear and concise and cover a wide-range of
> potential use-cases.

Great, thanks.

> It's not clear from the documentation if standalone Asio is
> supported. I think that would be a big user concern.

It is not. How is that a concern for a library that is part of boost?

<snip>

> > Do you think the library should be accepted as a Boost library?
>
> I'm going to vote REJECT.
>
> In my view, if there's room in Boost for a Redis library, it would
> be one that exposes a low-level protocol library similar to Beast or
> OpenSSL with its linked BIO pairs. This would enable the library to
> be I/O runtime-agnostic and would still permit the library author to
> provide an integration with Asio like it has today.

This library was built explicitly for ASIO, there are no plans for
making it usable for anything other than that. This means this
expectation can't be possibly fulfilled.

> Second, the value of this library being included in Boost isn't clear to me.
>
> No libraries currently in Boost would benefit from Aedis' inclusion.

That is true for many other libraries. But I respectfully disagree, I
would find it very valuable to have a complete Web stack in Boost,
alongside URL, Json, Beast and MySql and possibly Mustache.

<snip>

> it's safe to say that this library will be stable outside of Boost.

So what? I don't want to dispute your decision, but this is not a
reason to reject libraries AFAICS.

> While reddit is not necessarily a kind place that reflects the state of C++
> developers universally, the reddit thread:
> https://www.reddit.com/r/cpp/comments/10cnc99/candidate_boostaedis_review_starts_today_jan_15/
>
> did not seem to generate significant user interest in the library being
> accepted. Interestingly, most of the reddit thread is dedicated to discussing
> monolithic Boost. While largely non-productive (as most reddit conversations
> are),

IMO, using a Reddit thread to evaluate interest in Redis clients is
very misleading, in fact I don't think it can contribute anything to
the review.

Additionally, although C++ has a large number of Redis client projects
on github it comes far behind other languages in terms of popularity.
Bellow I accumulated the number of Github stars of the four most
popular clients of each language

 1. Java: 36k
 2. Node-js: 27k
 4. Go: 27k
 5. PHP: 17K
 3. Python: 13k
 6. C#: 7k
 7. C: 6k
 8. Ruby: 4k
 9. Rust: 3k
10. C++: 2k

This means the Redis community in C++ is itself very small
comparatively, let alone inside Boost. This is not to make a case for
my library, but to show the big potential this functionality has in
regard to users.

> we do see that many users are not excited about Yet Another Boost Library.

Well, then why are we reviewing another library?

<snip>

> Code Review Notes:
> * fix TODOs in the test suite before requesting formal Boost review
> * any parser facing the network needs fuzzing
> * the section comparing Aedis to redis++ should be first and foremost in the
>   docs as it's the first question people looking for a redis client are going to
>   ask

Will be done.

Thanks for taking the time to review the library.

Regards,
Marcelo