Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review Netty 5 buffer API #563

Open
danielcompton opened this issue Feb 15, 2021 · 2 comments
Open

Review Netty 5 buffer API #563

danielcompton opened this issue Feb 15, 2021 · 2 comments

Comments

@danielcompton
Copy link
Member

There's a proposed new buffer API for Netty 5: https://github.com/netty/netty-incubator-buffer-api/blob/070ffa26885ee078f364ad0334c9b177442f370b/RATIONALE.adoc. We should take a look at it and see if it will affect Aleph/Manifold and provide comment if so.

@arnaudgeiser
Copy link
Collaborator

arnaudgeiser commented Apr 3, 2023

I've been thinking for a while about what the transition to Netty 5 [1][2] might look like in Aleph. I didn't want to write anything before most the major issues have been closed.

I will deliberately provoke a bit of a debate here with my current take:

Long story short
We should not even try.

A little bit longer
Netty 5 ByteBuffer API changes will be impactful as we'll have to take care of the raw-stream? mode we expose to not break Aleph users. There are also the ChannelHandler changes that will need to be considered. While doable, I'm wondering if it worth the effort considering those changes will most likely impact a large part of the code base.

Netty 5 is actually a good opportunity to revisit the choices that have been made we regret today:

  • Strongly encapsulate the internals (we are always reluctant to touch any public functions that should not be used outside of Aleph. Today, those are marked as no-doc but still cannot be touched)
  • Remove some parts of the API that should not be used anymore
  • Revisit some of the defaults (multipart comes naturally to mind, but it might be also the case for the transport, netty parameters and so on...)
  • Do not use Dirigiste Executors by default and definitely move away from Dirigiste Pools.
  • Do not rely on manifold Deferred classes but on CompletableFuture instead
  • Do not rely on manifold to create the threads. The magical behavior is only understood by a few of us and users are surprised about those subtleties (tied to the previous point actually)
  • Still consider manifold Stream as a default implementation but make them returning CompletableFuture instead. I still think a push-based streams approach is the most appropriate and I don't have any alternative in mind. Maybe we should expose a protocol that "implementers" can rely on.
  • Still keep an asynchronous approach by default but consider virtual threads (aka Loom) as a first class citizen. Or consider the later as default as I expect some of you would prefer.
  • Make sure that client is fully clj-http compliant without any difference
  • Provide solutions for the issues that are currently open where it would be easy to provide a solution if backwards-compatibility was not required
  • (Fix everything that will be discovered when implementing HTTP/2 support and UNIX domain socket)

To summarize : It's a good opportunity to consider beth at that point but I would wait for HTTP/2 and the UNIX domain sockets to land :)

Thoughts?


EDIT

Yes, somehow it also implies considering multifold (aka manifold v2)

[1] : netty/netty#11347
[2] : https://netty.io/wiki/new-and-noteworthy-in-5.0.html

@KingMob
Copy link
Collaborator

KingMob commented Apr 5, 2023

I agree with almost all of this. Netty 5 introduces quite a bunch of changes, and I agree that new names would be warranted. Also, Loom virtual threads kind of throw a wrench in the entire event-driven paradigm. The value of libs like Manifold, and the event-driven parts of Netty, are in question.

Do not rely on manifold Deferred classes but on CompletableFuture instead

Agreed, although as of today, Deferreds now implement CompletionStage, courtesy of @figurantpp. 😄 But yes, a replacement Manifold should probably extend CompletableFuture to protocols, rather than roll its own.

I still think a push-based streams approach is the most appropriate and I don't have any alternative in mind

I think we should also consider reactive-streams' pull-based streams. On the upside, they neatly solve the backpressure problem automatically, as well as upstream cancellation, which we've never offered. On the downside, they complicate fan-out, and we'd be able to reuse very little existing code.

Ditto for vthreads.

Make sure that client is fully clj-http compliant without any difference

I need to finish that PR...

It's a good opportunity to consider beth

Beth/Bet/Vet would be the next letter in the Hebrew alphabet, but I think those names will be confusing, since they're also English words. We could keep the same letter, but switch languages, so it would be alif/alpha/olaf. And of course, omega and its variants is a possibility. For manifold's sequel, I thought "myriad" would be a good name...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants