Re: aedis review starts today

From: Christian Mazakas (christian.mazakas_at_[hidden])
Date: 2023-01-20 21:23:57

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.

In the ideal, this would be abstracted from me with an opt-in way of me handling
reconnecting manually but as long as users are comfortable copy-pasting from the
docs and example code, this is a non-issue.

The design emphasizes parsing directly into structures which I enjoy.

This is also the only library I've seen that puts PMR front-and-center in its
API which I do like. C++17 added PMR for a reason, it's good to see authors
taking advantage of this and I think it reflects the innovative spirit of Boost
that I've grown to appreciate and admire.

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.

Otherwise, the design is simple and effective.

> What is your evaluation of the implementation?

The implementation is actually quite clean and much like its presented
interfaces, is idiomatic Asio down to a T. I'm not overwhelmingly familiar with
the implementation details as I haven't spent a lot of time with it but after
looking at the connection_base class, I feel like I'd be able to debug this
library given my level of Asio expertise.

Debuggability is paramount when evaluating quality of implementation.

The code is laid out quite nicely and uses names effectively. I have no trouble
reading it.

> 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.

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

> What is your evaluation of the potential usefulness of the library?

It's useful in the sense that if you need a working Redis client, this fits the
bill.

While tightly integrating with Asio is nice, this library is almost a tad too
late. Having deployed Asio applications and servers, if I had needed a Redis
client I would've already been using one of the existing clients as noted on the
official Redis website.

This library would then require me gradually migrate our existing codebase
towards Aedis which requires buy-in from one's team and also from the business.
Depending on things like team size and how the company internally manages
dependencies and deployments, this is either a non-issue or can create a lot of
friction.

I've worked on a small team before and migrated us from nlohmann JSON to
Boost.JSON, much to the chagrin of my coworkers who didn't view the shift as
justified.

For greenfield projects looking to use Asio, this library is the most obvious
choice.

> Did you try to use the library? With what compiler? Did you have any problems?

I did not try to use the library aside from pulling it down and building and
running its examples and tests. I used modern compilers (gcc-12) and ran into
no problems.

> How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

I put in a moderate amount of effort. Historically I've never been too motivated
to add Redis to applications I've developed. I did, however, study the resp3
protocol to get a better feel for how the library could be designed around it.

> Are you knowledgeable about the problem domain?

I'm not very knowledgeable about in-memory key-value stores in general. I'm very
familiar with networking and using "advanced" APIs for working with myriad
protocols.

> 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.

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. Aedis
benefits somewhat from being in Boost as it can then evolve in lockstep with
Asio but unless Asio makes massive changes to its core Stream abstractions, it's
safe to say that this library will be stable outside of Boost.

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), we do see that many users are not excited about Yet Another Boost Library.

If there's strong user engagement somewhere else, I'll change my REJECT to an
ACCEPT but until then, I don't believe Boost itself benefits from Aedis and I
don't believe users benefit from Aedis being in Boost outside of those wishing
to use Boost as a package manager.

Instead, I'd opt into writing a vcpkg portfile and a Conan recipe for this
library and distribute it that way. Users will still undoubtedly benefit from
this library.

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

- Christian