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

Skip serialization of jvm invocation in ark #370

Merged
merged 18 commits into from
Mar 8, 2019

Conversation

QilongZhang
Copy link
Contributor

@QilongZhang QilongZhang commented Mar 7, 2019

@QilongZhang QilongZhang requested review from caojie09, ujjboy, glmapper and straybirdzls and removed request for caojie09 March 7, 2019 05:08
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #370 into master will increase coverage by 1.93%.
The diff coverage is 73.52%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #370      +/-   ##
=========================================
+ Coverage   66.56%   68.5%   +1.93%     
=========================================
  Files         130     132       +2     
  Lines        3739    3801      +62     
  Branches      511     514       +3     
=========================================
+ Hits         2489    2604     +115     
+ Misses        960     901      -59     
- Partials      290     296       +6
Impacted Files Coverage Δ
...untime/service/helper/ReferenceRegisterHelper.java 72.72% <ø> (ø) ⬆️
...itializer/SofaRuntimeSpringContextInitializer.java 98.27% <ø> (ø) ⬆️
...me/spring/factory/AbstractContractFactoryBean.java 88.31% <0%> (+34.56%) ⬆️
...fa/runtime/service/client/ReferenceClientImpl.java 28.84% <100%> (+4.35%) ⬆️
...ofa/runtime/spring/factory/ServiceFactoryBean.java 73.68% <100%> (+7.01%) ⬆️
...a/runtime/spring/factory/ReferenceFactoryBean.java 85.18% <100%> (+13.18%) ⬆️
...tegration/invoke/DynamicJvmServiceProxyFinder.java 63.2% <46.15%> (-2.75%) ⬇️
...a/runtime/service/binding/JvmBindingConverter.java 70% <70%> (ø)
...untime/spring/ServiceBeanFactoryPostProcessor.java 81.05% <73.33%> (+4.61%) ⬆️
...e/spring/ReferenceAnnotationBeanPostProcessor.java 82.81% <76.92%> (+16.63%) ⬆️
... and 12 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 d46d7b3...052d946. Read the comment docs.

@QilongZhang QilongZhang added this to the 3.1.3 milestone Mar 7, 2019
@QilongZhang QilongZhang added the enhancement New feature or request label Mar 7, 2019
} finally {
pushThreadContextClassLoader(tcl);
}
} else if (TOSTRING_METHOD.equalsIgnoreCase(targetMethod.getName())
Copy link
Member

Choose a reason for hiding this comment

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

这里的 else 可以去掉下,可读性更强点

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已改。

@@ -127,6 +127,10 @@
<xsd:attribute name="registry" type="xsd:string" use="optional"/>
<xsd:attribute name="serialize-type" type="xsd:string" use="optional"/>
</xsd:complexType>
<!-- binding.jvm im reference -->
<xsd:complexType name="BBindingJvmReference">
<xsd:attribute name="serialize" type="xsd:boolean" default="false" use="optional"/>
Copy link
Member

Choose a reason for hiding this comment

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

这个为啥默认是false

Copy link
Contributor Author

@QilongZhang QilongZhang Mar 7, 2019

Choose a reason for hiding this comment

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

reference 默认不走序列化,service 默认走序列化. 只有两者都为 false 是,才会走直接调用模式。

服务端如果设置不走序列化,需要将相关类导出,而客户端无需变更代码。

只有当引用服务的应用配置了 deny-import 相关类,此时,客户端可能需要强制设置走序列化方式.

@@ -127,6 +127,10 @@
<xsd:attribute name="registry" type="xsd:string" use="optional"/>
<xsd:attribute name="serialize-type" type="xsd:string" use="optional"/>
</xsd:complexType>
<!-- binding.jvm im reference -->
Copy link
Contributor

Choose a reason for hiding this comment

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

im 是什么意思?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo , 已改。

@@ -33,8 +33,18 @@
*/
public static BindingType JVM_BINDING_TYPE = new BindingType("jvm");
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的 "jvm" 字符串也换成下面的 XmlConstants 常量吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已替换

@QilongZhang QilongZhang merged commit b1b0f0e into sofastack:master Mar 8, 2019
@QilongZhang QilongZhang mentioned this pull request Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants