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

Support @SofaService to annotated on method and refactor related code. #288

Merged
merged 11 commits into from
Nov 29, 2018

Conversation

QilongZhang
Copy link
Contributor

@QilongZhang QilongZhang commented Nov 24, 2018

@codecov
Copy link

codecov bot commented Nov 24, 2018

Codecov Report

Merging #288 into master will increase coverage by 2.64%.
The diff coverage is 79.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   60.19%   62.83%   +2.64%     
==========================================
  Files          73       77       +4     
  Lines        1781     1975     +194     
  Branches      203      230      +27     
==========================================
+ Hits         1072     1241     +169     
- Misses        557      569      +12     
- Partials      152      165      +13
Impacted Files Coverage Δ
...ntime/spring/parser/ReferenceDefinitionParser.java 50% <ø> (ø) ⬆️
.../com/alipay/sofa/infra/utils/SOFABootEnvUtils.java 83.33% <ø> (ø) ⬆️
...pring/parser/AbstractContractDefinitionParser.java 58.62% <ø> (ø) ⬆️
...ng/configuration/SofaRuntimeAutoConfiguration.java 80% <ø> (-14.6%) ⬇️
...ofa/runtime/spring/factory/ServiceFactoryBean.java 66.66% <0%> (+8.33%) ⬆️
...runtime/spring/parser/ServiceDefinitionParser.java 66.66% <100%> (ø) ⬆️
...me/spring/factory/AbstractContractFactoryBean.java 56.75% <54.54%> (+9.13%) ⬆️
...ofa/runtime/spring/bean/SofaBeanNameGenerator.java 64.28% <64.28%> (ø)
...e/spring/ReferenceAnnotationBeanPostProcessor.java 67.64% <71.42%> (ø)
...untime/spring/ServiceBeanFactoryPostProcessor.java 76.72% <76.72%> (ø)
... and 11 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 0197fd0...abaec33. Read the comment docs.

@straybirdzls
Copy link
Member

@QilongZhang There is a huge decline on the code cov, please keep us noticed when it get resolved.

@QilongZhang
Copy link
Contributor Author

To support to this feature, it would produce an incompatible point. In previous version, publishing multi sofa service with same interface type and unique-id would only log warning message. However, now it would lead a fatal exception if spring bean override strategy is not configured as TRUE.

@QilongZhang
Copy link
Contributor Author

To support to this feature, it would produce an incompatible point. In previous version, publishing multi sofa service with same interface type and unique-id would only log warning message. However, now it would lead a fatal exception if spring bean override strategy is not configured as TRUE.

After some discussion, we decide to fix this incompatible point and log some error message to notice users

2018-11-27 11:09:37,351 ERROR main                             - SofaService was already registered: ServiceFactoryBean#com.alipay.sofa.runtime.beans.service.SampleService

GenericApplicationContext ctx = sofaModuleProperties.isPublishEventToParent() ? new GenericApplicationContext(
beanFactory) : new SofaModuleApplicationContext(beanFactory);
SofaRuntimeApplicationListener.initApplicationContext(ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

这里直接调用 sofa-runtime 的方法是不是有点奇怪,isle 不应该强耦合 sofa-runtime 吧?在我的理解,如果 sofa-runtime 需要提供一个钩子供 isle 回调,那么其他 starter 是不是也可能有这个需求,是不是能做成更通用的方案呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caojie09 SOFAIsle 和 sofa-runtime 一直是强耦合的. 这里的编码也不是很友好,考虑换一种方式,采用类似 SofaModuleBeanFactoryPostProcessor 的白名单形式也行。

Copy link
Member

@straybirdzls straybirdzls Nov 28, 2018

Choose a reason for hiding this comment

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

@caojie09 @QilongZhang As discussed offline, I prefer that we should add listener on isle(and mvc) to make the code more flexible.

* @author xuanbei 18/5/9
*/
public class ServiceAnnotationBeanPostProcessor implements BeanPostProcessor, Ordered,
ApplicationContextAware {
public class ServiceAnnotationBeanPostProcessor implements BeanPostProcessor, Ordered {
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this class to make it more clear?

Copy link
Contributor Author

@QilongZhang QilongZhang Nov 29, 2018

Choose a reason for hiding this comment

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

ok, rename it to ReferenceAnnotationBeanPostProcessor

if (StringUtils.isEmpty(uniqueId)) {
return SERVICE_BEAN_NAME_PREFIX + interfaceName;
}
return SERVICE_BEAN_NAME_PREFIX + interfaceName + ":" + uniqueId;
Copy link
Member

@straybirdzls straybirdzls Nov 28, 2018

Choose a reason for hiding this comment

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

Duplicate logics here, should directly call following generateSofaServiceBeanName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not actually, parameter type is different

SofaReference sofaReference,
Class<?> parameterType,
ConfigurableListableBeanFactory beanFactory) {
Assert.isTrue("jvm".equals(sofaReference.binding().bindingType()),
Copy link
Member

Choose a reason for hiding this comment

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

Use JvmBinding.JVM_BINDING_TYPE.getType() here instead.

Copy link
Member

Choose a reason for hiding this comment

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

And why only support "jvm" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not clear whether it is suitable to support RPC, maybe it would be covered according to actually demand.


/**
* @author qilong.zql
* @since 2.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

since 2.3.0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix.

returnType = ClassUtils.forName(methodMetadata.getReturnTypeName(), null);
declaringClass = ClassUtils.forName(methodMetadata.getDeclaringClassName(), null);
for (Method m : declaringClass.getDeclaredMethods()) {
if (!m.getName().equals(beanId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

when use @Bean("customBeanName"), beadId not equals methodName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix.

@straybirdzls straybirdzls merged commit 50a95ec into sofastack:master Nov 29, 2018
@ujjboy ujjboy added this to the 3.1.0 milestone Nov 30, 2018
@QilongZhang QilongZhang deleted the support_issue_277 branch December 26, 2018 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants