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

Add support for http2 prior knowledge mode. (#164) #179

Merged
merged 5 commits into from
Jun 15, 2018
Merged

Add support for http2 prior knowledge mode. (#164) #179

merged 5 commits into from
Jun 15, 2018

Conversation

jjtyro
Copy link
Contributor

@jjtyro jjtyro commented Jun 14, 2018

Fixes #164

Add some code for http transport to support http2 Prior-Knowledge mode connection.

@jjtyro
Copy link
Contributor Author

jjtyro commented Jun 14, 2018

What means :
"[0K$ sh ./tools/check_format.sh
Please commit your change before run this shell, un commit files:
M extension-impl/remoting-http/src/main/java/com/alipay/sofa/rpc/transport/http/Http2ClientInitializer.java"
It have been commit in branch at my side

@JervyShi
Copy link
Member

@jjtyro See Contributing , you could run mvn clean compile to make sure your changes have been formated.

@ujjboy ujjboy added the optimization Code optimization label Jun 14, 2018
@ujjboy ujjboy added this to the 5.4.1 milestone Jun 14, 2018
@ujjboy ujjboy added this to To do in v5.4.x via automation Jun 14, 2018
@jjtyro
Copy link
Contributor Author

jjtyro commented Jun 14, 2018

@JervyShi OhOhOh, I ran 'mvn clean compile' and then I saw one line of code changed and I deleted the extra space.

@jjtyro
Copy link
Contributor Author

jjtyro commented Jun 14, 2018

@JervyShi oraclejdk8 and openjdk6 successed. openjdk7 failed? why?

@JervyShi
Copy link
Member

@jjtyro You can figure it out or just retry.

@leizhiyuan
Copy link
Contributor

I run jdk7 again

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #179 into master will increase coverage by 0.12%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #179      +/-   ##
============================================
+ Coverage     68.89%   69.02%   +0.12%     
- Complexity      951      955       +4     
============================================
  Files           341      341              
  Lines         14183    14194      +11     
  Branches       2276     2278       +2     
============================================
+ Hits           9772     9797      +25     
+ Misses         3241     3232       -9     
+ Partials       1170     1165       -5
Impacted Files Coverage Δ Complexity Δ
...in/java/com/alipay/sofa/rpc/common/RpcOptions.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...ofa/rpc/transport/http/Http2ClientInitializer.java 73.77% <91.66%> (+3.77%) 8 <2> (+2) ⬆️
...pay/sofa/rpc/transport/ClientTransportFactory.java 73.84% <0%> (-3.08%) 0% <0%> (ø)
...ay/sofa/rpc/client/AllConnectConnectionHolder.java 63.12% <0%> (+0.27%) 0% <0%> (ø) ⬇️
.../alipay/sofa/rpc/registry/local/LocalRegistry.java 57.83% <0%> (+0.6%) 26% <0%> (+1%) ⬆️
.../rpc/transport/http/Http2ServerChannelHandler.java 75.63% <0%> (+0.84%) 25% <0%> (+1%) ⬆️
...ava/com/alipay/sofa/rpc/config/RegistryConfig.java 36% <0%> (+1%) 0% <0%> (ø) ⬇️
...com/alipay/sofa/rpc/context/RpcRuntimeContext.java 90.24% <0%> (+1.21%) 0% <0%> (ø) ⬇️
...ofa/rpc/registry/zk/ZookeeperOverrideObserver.java 15.27% <0%> (+1.38%) 0% <0%> (ø) ⬇️
...ofa/rpc/registry/zk/ZookeeperProviderObserver.java 68.51% <0%> (+1.85%) 0% <0%> (ø) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53569c0...6b50362. Read the comment docs.

/**
* Whether the Http2 Cleartext protocol client uses Prior Knowledge to start Http2
*/
public static final String TRANSPORT_H2C_USE_PRIOR_KNOWLEDGE = "transport.h2c.use_prior_knowledge";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to transport.client.h2c.usePriorKnowledge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@@ -258,6 +258,8 @@ PS:大家也看到了,本JSON文档是支持注释的,而标准JSON是不支
"compress.open": false,
// 开启压缩的大小基线
"compress.size.baseline": 2048,
//Whether the Http2 Cleartext protocol client uses Prior Knowledge to start Http2
"transport.h2c.use_prior_knowledge": false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default value can set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set false to keep the original default behavior of using upgrade h2c

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It 's ok to set true because we know exactly the protocol of provider is h2c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I changed it to true.

@@ -74,7 +83,11 @@ public void initChannel(SocketChannel ch) throws Exception {
if (RpcConstants.PROTOCOL_TYPE_H2.equals(protocol)) {
configureSsl(ch);
} else if (RpcConstants.PROTOCOL_TYPE_H2C.equals(protocol)) {
configureClearText(ch);
if (!useH2cPriorKnowledge) {
configureClearText(ch);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configureClearTextWithHttpUgrade()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will modify it.

if (!useH2cPriorKnowledge) {
configureClearText(ch);
} else {
configureClearTextPriorKnowledge(ch);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configureClearTextWithPriorKnowledge()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will modify it.

private static class PrefaceFrameWrittenEventHandler extends ChannelInboundHandlerAdapter {
@Override
public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
System.out.println("User Event Handler :" + evt.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use LOGGER instead of System.out.println

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, it is learned from previous code's UserEventLogger, I will change it

@sofastack sofastack deleted a comment from khotyn Jun 14, 2018
RpcConfigs.putValue("transport.client.h2c.usePriorKnowledge", false);
ActivelyDestroyTest.adBeforeClass();
}

Copy link
Member

@ujjboy ujjboy Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add @AfterClass method for reset default value of transport.client.h2c.usePriorKnowledge, and use RpcOptions.TRANSPORT_CLIENT_H2C_USE_PRIOR_KNOWLEDGE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

@ujjboy ujjboy merged commit 3a0b068 into sofastack:master Jun 15, 2018
v5.4.x automation moved this from To do to Done Jun 15, 2018
@ujjboy ujjboy changed the title add support for http2 prior knownledge mode (#164) Add support for http2 prior knowledge mode. (#164) Jun 15, 2018
@jjtyro jjtyro deleted the add_support_http2_prior_knowledge_feature branch June 15, 2018 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Code optimization
Projects
No open projects
v5.4.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants