You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2022/03/21 03:23:01 UTC

[GitHub] [james-project] chibenwa opened a new pull request #924: JAMES-3715 Migrates to Netty4

chibenwa opened a new pull request #924:
URL: https://github.com/apache/james-project/pull/924


   Combines https://github.com/apache/james-project/pull/886 and its fixes https://github.com/apache/james-project/pull/908 together as discussed on the mailing list cf https://www.mail-archive.com/server-dev@james.apache.org/msg71688.html
   
   Also include a fix mentionned by @glonosss regarding child options. It thus might be worth to do another performance measurment as the latencies we see might well be due to TCP_NODELAY not being positioned on the child channels.
   
   Comments welcome BTW!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1075816011


   JAMES-3715 Improved failure messages on bind/unbind failures.
   1. Wrap netty exception from different call stack - which netty is just blindly rethrowing in the new call stack. This issue is discussed more here: https://github.com/netty/netty/issues/2597
   2. On bind, set better failure message to promise that includes the address that caused the failure. Without this you just get 'java.net.BindException: Address already in use: bind'.
   3. Changed 'Future<?>' to 'Future<Void>' to cause less issues when manipulating futures (found situations where couldn't assign 'Future<?>' to another 'Future<?>').
   https://github.com/glennosss/james-project/commit/d648c57f3066f9d90f15f8001ae4d3c7944d7697


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss edited a comment on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss edited a comment on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1075672967


   [JAMES-3715 Minor fix in case someone decides to share 'EventLoopGroup ManagerDefault' between servers](https://github.com/glennosss/james-project/commit/0d65b007a26af7fc3c1bff85de1f33e7761d54a7)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1075672967


   JAMES-3715 Minor fix in case someone decides to share 'EventLoopGroup ManagerDefault' between servers - https://github.com/glennosss/james-project/commit/0d65b007a26af7fc3c1bff85de1f33e7761d54a7


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1073696926


   ## Netty 4
   
   ![Screenshot from 2022-03-21 16-49-11](https://user-images.githubusercontent.com/6928740/159237873-43e75d69-5a3a-4337-b100-22061f07fa84.png)
   
   CPU flame graph: 
   
   [netty4-final.zip](https://github.com/apache/james-project/files/8314457/netty4-final.zip)
   
   Memory flame graph:
   
   [netty4-mem.zip](https://github.com/apache/james-project/files/8314460/netty4-mem.zip)
   
   ## Netty 3
   
   ![Screenshot from 2022-03-17 16-09-44](https://user-images.githubusercontent.com/6928740/158776296-e3622b3f-fb99-4895-aede-98372cfabc39.png)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss edited a comment on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss edited a comment on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1075816011


   [JAMES-3715 Improved failure messages on bind/unbind failures.](https://github.com/glennosss/james-project/commit/d648c57f3066f9d90f15f8001ae4d3c7944d7697)
   1. Wrap netty exception from different call stack - which netty is just blindly rethrowing in the new call stack. This issue is discussed more here: https://github.com/netty/netty/issues/2597
   2. On bind, set better failure message to promise that includes the address that caused the failure. Without this you just get 'java.net.BindException: Address already in use: bind'.
   3. Changed 'Future<?>' to 'Future<Void>' to cause less issues when manipulating futures (found situations where couldn't assign 'Future<?>' to another 'Future<?>').


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa edited a comment on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074768865


   > FYI Also includes a fix for TCP_NODELAY still wasn't being set properly (configureBootstrap() was being run after socket was bound).
   
   Crap my bad!
   
   > Please cherry-pick: https://github.com/glennosss/james-project/commit/19ca9349f5a26e114c7acc1121bdc208be27e622
   
   I will review that carefully tomorrow.
   
   Don't you opening it as a pull request?
   
   This will ease discussions for sure.
   
   Also, splitting it into smaller commits would make it significantly easier to adopt! I see 5 bullet points in your commit, this should likely be 5 different commits...
   
   Also avoid changesets on wal files like `BasicChannelInboundHandler`. I guess it is a line termination fix. Changes those things if needed should be done in separate commits.
   
   Also beware of licensing of `EventLoopGroupManager`, the `2021 Kaxu Systems [All rights reserved]` don't seem compatible to me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074786864


   Created pull request as requested!
   
   Would you like me to split this commit up a bit, or just leave it for now?
   
   My apologies for sending in such a large request - I was trying to clean stuff up to fit into my own project and was finding it near impossible to manage/share EventLoopGroups between different netty servers - so I created https://github.com/glennosss/james-project/blob/netty4-combo/protocols/netty/src/main/java/org/apache/james/protocols/netty/EventLoopGroupManager.java#L34 to solve my and what I believe would be many other peoples biggest problem. Cloud servers these days are often so resource constrained - and splitting this out allows people to easily share thread pools between all their netty servers.
   
   I'm also doing a lot of work with Reactor Netty - seems to be the way everyones going these days - and wanted to have good support for async and starting/stopping the server in a very configurable way - so added https://github.com/glennosss/james-project/blob/netty4-combo/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolServer.java#L88
   
   I fixed the licensing at the top of those files - my bad!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074019559


   :greeen_apple: . Netty4 migration is open for so long that I will likely merge it tommorrow morning but I will keep on answering comments might there be some!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074760329


   Please cherry-pick: https://github.com/glennosss/james-project/commit/19ca9349f5a26e114c7acc1121bdc208be27e622
   From branch: https://github.com/glennosss/james-project/tree/netty4-combo
   
   Several fixes and improvements to make starting and managing netty servers easier - specifically:
   1. Created 'EventLoopGroupManager' interface - see interface javadoc for more information - allows much better control over starting/sharing EventLoopGroups - https://github.com/glennosss/james-project/blob/netty4-combo/protocols/netty/src/main/java/org/apache/james/protocols/netty/EventLoopGroupManager.java
   2. Updated 'Encryption' to support old static methods, but added additional static methods which add Netty SslContext support - see 'Encryption' javadoc for more information - https://github.com/glennosss/james-project/blob/netty4-combo/protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
   3. See 'ProtocolServer' javadoc - now supports async bind/unbind, control over how long unbind takes, and added unbindNow() to help with instantly stopping server for unit tests - https://github.com/glennosss/james-project/blob/netty4-combo/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolServer.java
   4. Updated 'NettyServer', 'AbstractAsyncServer' to support proper construction using Factory/Builder approach - making it much easier to method chain and build a server.
   5. Updated 'AbstractConfigurableAsyncServer' to support Factory/Builder approach - also 'bossThreads' and 'keepalive' parameters are now configurable.
   
   FYI Also includes a fix for TCP_NODELAY still wasn't being set properly (`configureBootstrap()` was being run after socket was bound).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] glennosss commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
glennosss commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1076071648


   Please see https://github.com/apache/james-project/pull/929


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa merged pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #924:
URL: https://github.com/apache/james-project/pull/924


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa edited a comment on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074768865


   > FYI Also includes a fix for TCP_NODELAY still wasn't being set properly (configureBootstrap() was being run after socket was bound).
   
   Crap my bad!
   
   > Please cherry-pick: https://github.com/glennosss/james-project/commit/19ca9349f5a26e114c7acc1121bdc208be27e622
   
   I will review that carefully tomorrow.
   
   Don't you opening it as a pull request?
   
   This will ease discussions for sure.
   
   Also, splitting it into smaller commits would make it significantly easier to adopt! I see 5 bullet points in your commit, this should likely be 5 different commits...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org

[GitHub] [james-project] chibenwa commented on pull request #924: JAMES-3715 Migrates to Netty4

Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #924:
URL: https://github.com/apache/james-project/pull/924#issuecomment-1074768865


   > FYI Also includes a fix for TCP_NODELAY still wasn't being set properly (configureBootstrap() was being run after socket was bound).
   
   Crap my bad!
   
   > Please cherry-pick: https://github.com/glennosss/james-project/commit/19ca9349f5a26e114c7acc1121bdc208be27e622
   
   I will review that carefully tomorrow.
   
   Don't you opening it as a pull request?
   
   This will ease discussions for sure.
   
   Also, splitting it into smaller commits would make it significantly easier to adopt!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org