This repository was archived by the owner on Oct 13, 2024. It is now read-only.
[SCB-1593] Use Netty native transport improve performance#593
Merged
WillemJiang merged 9 commits intoapache:masterfrom Nov 15, 2019
Merged
[SCB-1593] Use Netty native transport improve performance#593WillemJiang merged 9 commits intoapache:masterfrom
WillemJiang merged 9 commits intoapache:masterfrom
Conversation
… versions of gPRC and Netty
…/BSD (since 4.1.11)
WillemJiang
reviewed
Nov 15, 2019
| private String clientCert; | ||
|
|
||
| @Value("${alpha.feature.native:false}") | ||
| private boolean nettyTransport; |
Member
There was a problem hiding this comment.
How about Rename the configuration setting to nativetransport.
WillemJiang
reviewed
Nov 15, 2019
| private Class<? extends ServerChannel> selectorServerChannel() { | ||
| Class<? extends ServerChannel> channel = NioServerSocketChannel.class; | ||
| if (serverConfig.isNettyTransport()) { | ||
| if (OSInfo.isLinux()) { |
Member
There was a problem hiding this comment.
It's better to add some Info log to tell user which kind of ServerSocketChannel that Alpha choices.
Member
Author
There was a problem hiding this comment.
I have marked the location of the log output below.
coolbeevip
commented
Nov 15, 2019
| channel = KQueueServerSocketChannel.class; | ||
| } | ||
| } | ||
| LOG.info("Netty channel type is " + channel.getSimpleName()); |
coolbeevip
commented
Nov 15, 2019
| group = new KQueueEventLoopGroup(nThreads); | ||
| } | ||
| } | ||
| LOG.info("Netty event loop group is " + group.getClass().getSimpleName()); |
WillemJiang
reviewed
Nov 15, 2019
| @@ -56,7 +56,7 @@ public class GrpcServerConfig { | |||
| private String clientCert; | |||
|
|
|||
| @Value("${alpha.feature.native:false}") | |||
Member
There was a problem hiding this comment.
We also need to update this part.
zhfeng
reviewed
Nov 15, 2019
alpha/alpha-server/src/main/java/org/apache/servicecomb/pack/alpha/server/GrpcStartable.java
Show resolved
Hide resolved
WillemJiang
reviewed
Nov 15, 2019
| # limitations under the License. | ||
|
|
||
| ciphers = ECDHE-RSA-AES128-GCM-SHA256,ECDHE-RSA-AES256-GCM-SHA384,ECDHE-ECDSA-AES128-SHA256 | ||
| ciphers = ECDHE-RSA-AES128-GCM-SHA256,ECDHE-RSA-AES256-GCM-SHA384 |
Member
There was a problem hiding this comment.
It's better to add a link of the context why do we remove the cipher to help others trace the issue.
Member
Author
There was a problem hiding this comment.
I added the link in the comment of the commit. Do I still need to explain it in the source file?
coolbeevip
commented
Nov 15, 2019
WillemJiang
approved these changes
Nov 15, 2019
zhfeng
approved these changes
Nov 15, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.