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

[Poll] Shading Netty #1168

Closed
trustin opened this issue Apr 27, 2018 · 8 comments
Closed

[Poll] Shading Netty #1168

trustin opened this issue Apr 27, 2018 · 8 comments

Comments

@trustin
Copy link
Member

trustin commented Apr 27, 2018

Related issues:

We must decide to shade Netty or not in our shaded JARs.

Why? Because we cannot shade Netty as long as we expose Netty classes in our public API.

Why? Because otherwise our public API will be different between our shaded JARs and unshaded JARs.

Therefore, if we decide to shade Netty in our shaded JARs, we must hide all references to Netty classes from our public API. This will involve some breaking changes such as:

  • Provide some abstraction layer for core Netty classes such as EventLoop and ChannelOption
  • Fork other Netty classes we expose in our public API, such as:
    • AsciiString - affects HttpHeaders a lot
    • AttributeMap - affects RequestContext.attrs()
    • Headers - affects HttpHeaders a lot, because HttpHeaders extends Headers
    • ByteBuf - affects ByteBufHttpData
  • Make sure tcnative and epoll works even after shading.

Some may argue that such a large change can't be justified, given that Netty 4.1 has been fairly stable recently and 4.0 reached at its end of life.

In my opinion, there are three options for this matter:

  1. Shade Netty completely.
    • This is the most safe option, but requires all the changes mentioned above.
  2. Do not shade netty-common, netty-buffer, netty-transport-* and netty-codec. Shade other Netty dependencies such as netty-codec-http, netty-codec-http2 and netty-resolver-*
    • This will still break if mixed with something that depends on Netty 4.0.
    • This will still break if Netty makes a breaking change in unshaded Netty dependencies in 4.1, although I believe it is not very likely.
    • Breaking changes in Armeria will be kept minimal, because we can still refer to AsciiString, AttributeMap, ByteBuf, EventLoop, ChannelOption and Header.
  3. Do not shade Netty at all.
    • This may also be fine. A user could just shade Netty when building his or her distribution.

Please feel free to ask questions or discuss about the pros and cons of each choice. Please add the following reactions to this issue to cast a vote:

  • 😄 for (1) Shade Netty completely.
  • 🎉 for (2) Shade Netty partially. We will discuss later again about which Netty artifact should be and should not be shaded.
  • ❤️ for (3) Do not shade Netty at all.

This poll will be open for about a week.

@trustin
Copy link
Member Author

trustin commented Apr 27, 2018

/cc @anuraaga @arkadius @imasahiro @rkapsi @taicki and anyone interested

@trustin
Copy link
Member Author

trustin commented Apr 27, 2018

I voted to all three options so that the voters can cast conveniently. I'll cast/uncast mine when I close the poll.

@anuraaga
Copy link
Collaborator

Thanks for setting up the poll :)

Out of curiosity, do you expect a performance impact from the abstraction? I guess it will mostly inline and be minimal and want to confirm with your feeling.

Also, since ByteBuf is advanced and unsafe, what do you think about leaving it as is? If an advanced user runs into issues they can shade themselves. I personally use it a lot for performance reasons and would be sad to be unable to use it with Netty APIs cleanly due to an abstraction. The others don't seem like a problem (actually EventLoop is always a pain in tests and would be a welcome removal :) )

@trustin
Copy link
Member Author

trustin commented Apr 28, 2018

Out of curiosity, do you expect a performance impact from the abstraction? I guess it will mostly inline and be minimal and want to confirm with your feeling.

I think there will be performance impact if we replace AsciiString with something else because Netty HTTP 1/2 codecs handle it specially. We can of course fork Netty's HTTP codecs although it's not ideal.

Also, since ByteBuf is advanced and unsafe, what do you think about leaving it as is? If an advanced user runs into issues they can shade themselves.

That would fall into the option (2) - shading Netty partially, in this case 'everything except netty-common and netty-buffer.'

@rkapsi
Copy link

rkapsi commented Apr 28, 2018

To give a bit context for my selfish vote. Our application is a Netty application (an Software Edge LB). Many years ago we managed the lifecycle of that application ourselves. But then a need for internal APIs (think dashboards) emerged and we didn't want to build these higher level abstractions on Netty. The Servlet API and Jersey (v1) are kind of nice for that stuff.

So we started to embed our Netty application into a Servlet Container. Initially Jetty Embedded, then Dropwizard, Spring Boot. Our main() method would bootstrap these containers which in turn bootstrap the Netty application through their lifecycle events.

That is all great but we'd like to make a leap and start looking into topics such as SO_REUSEPORT. Something we can't really do as long as any part of the JVM instance uses traditional networking APIs.

This is where Armeria comes into play. It has the nice high level abstractions but is also built on Netty and we can leverage all that goodness across our application.

In that respect unshaded+access to all underlying guts would work best for us and we have no expectation in regards to API breakage. We like APIs that allow to shoot ourselves. It's way better than having to re-implement let's say ServerBuilder and its internals.

@trustin
Copy link
Member Author

trustin commented Apr 29, 2018

@rkapsi We can expose all Netty channel options via some abstraction layer even if we shaded Netty, so no worries :-)

@trustin
Copy link
Member Author

trustin commented May 8, 2018

Gentle reminder - I'll close this poll this evening (GMT+9).

@trustin
Copy link
Member Author

trustin commented May 9, 2018

Closing the poll:

  • 😄 1
  • 🎉 5
  • ❤️ 10

Let us not shade Netty.

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

No branches or pull requests

3 participants