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

fix default setting #4955

Merged
merged 533 commits into from
Sep 10, 2019
Merged

fix default setting #4955

merged 533 commits into from
Sep 10, 2019

Conversation

brucelwl
Copy link
Contributor

private AnnotatedMethodElement  --> public AnnotatedMethodElement  
add fields getter/ setter

DubboConfigBindingBeanPostProcessor -- > remove fixed settings 
 private boolean ignoreUnknownFields = true;
 private boolean ignoreInvalidFields = true;

htynkn and others added 30 commits April 26, 2018 15:07
* fixes #1089, make ExecutionDispatcher meets dubbo-user-book

* remove heartbeat condition
* use three different kinds of cache factory to increase test coverages

* add unit test cases for dubbo-filter module

* add copyright and made small refactor

* make sure Jcache will exceed expired period
…faceConfig should be in setter instead of getter (#1732)
* finish unit test for AbstractMethodConfig

* finish unit test for AbstractReferenceConfig

* move to right package

* finish unit test for AbstractReferenceConfig

* finish unit test for AbstractMethodConfig

* finish unit test for AbstractReferenceConfig

* move to right package

* finish unit test for AbstractReferenceConfig

* unit test for AbstractServiceConfig, also fix logic issue in setListener/getListener
… less 2 (#1759)

* fix #934 use loadBalance policy to choose invoke when providers less than 2
* fix #1756, clear mock invocation after invoking
* unit test for ApplicationConfig

* fix typo

* unit test for ArgumentConfig

* unit test for ConsumerConfig

* unit test for MethodConfig

* unit test for ModuleConfig

* unit test for MonitorConfig

* unit test for ProtocolConfig

* unit test for ApplicationConfig

* fix typo

* unit test for ArgumentConfig

* unit test for ConsumerConfig

* unit test for MethodConfig

* unit test for ModuleConfig

* unit test for MonitorConfig

* unit test for ProtocolConfig

* unit test for ProviderConfig

* make test stable
@htynkn
Copy link
Member

htynkn commented Sep 2, 2019

Could you provide more detail how this change?
Are you trying to fix some issue?

@brucelwl
Copy link
Contributor Author

brucelwl commented Sep 2, 2019

Could you provide more detail how this change?
Are you trying to fix some issue?
@htynkn
我需要通过AnnotatedMethodElement 获取里面的注解信息及引用对象,但是这个类是私有的, 将他改成public,比较合理.
还有 DubboConfigBindingBeanPostProcessor 将这两个属性值固定死了, 无论外部bean怎么配置都会在这里改变,所以将这两个属性改成局部变量比较合理
private boolean ignoreUnknownFields = true;
private boolean ignoreInvalidFields = true;

@codecov-io
Copy link

codecov-io commented Sep 4, 2019

Codecov Report

Merging #4955 into 2.6.x will decrease coverage by 0.02%.
The diff coverage is 44.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.6.x    #4955      +/-   ##
============================================
- Coverage     47.38%   47.35%   -0.03%     
+ Complexity     4392     4391       -1     
============================================
  Files           562      562              
  Lines         25053    25052       -1     
  Branches       4426     4426              
============================================
- Hits          11871    11864       -7     
- Misses        11357    11361       +4     
- Partials       1825     1827       +2
Impacted Files Coverage Δ Complexity Δ
...nnotation/DubboConfigBindingBeanPostProcessor.java 85.41% <100%> (+8.63%) 12 <0> (ø) ⬇️
...nnotation/AnnotationInjectedBeanPostProcessor.java 62.74% <37.5%> (-3.01%) 21 <0> (ø)
...onfig/spring/extension/SpringExtensionFactory.java 75.67% <0%> (-8.11%) 9% <0%> (ø)
...bo/remoting/transport/netty/NettyCodecAdapter.java 53.12% <0%> (-1.57%) 3% <0%> (ø)
.../com/alibaba/dubbo/monitor/dubbo/DubboMonitor.java 89.71% <0%> (-0.94%) 15% <0%> (-1%)

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 36fc6c5...03e6e7a. Read the comment docs.

@htynkn
Copy link
Member

htynkn commented Sep 4, 2019

Could you provide more detail how this change?
Are you trying to fix some issue?
@htynkn
我需要通过AnnotatedMethodElement 获取里面的注解信息及引用对象,但是这个类是私有的, 将他改成public,比较合理.
还有 DubboConfigBindingBeanPostProcessor 将这两个属性值固定死了, 无论外部bean怎么配置都会在这里改变,所以将这两个属性改成局部变量比较合理
private boolean ignoreUnknownFields = true;
private boolean ignoreInvalidFields = true;

1.能描述一下相关场景吗?getInjectedMethodObjectsMap和getInjectedFieldObjectsMap可以满足需求吗?我们需要比较充足的理由再调整一个方法的访问级别,同时getInjectedObjects方法的返回参数可能是不兼容的变更,不是很建议这么操作

2.DubboConfigBindingBeanPostProcessor中的这两个值提供了对应的setter方法,如果是spring配置,也是可以配置property变更的,能描述你使用中的具体问题吗?

@brucelwl
Copy link
Contributor Author

brucelwl commented Sep 4, 2019

@htynkn
1 因为我需要统计项目中引用了哪些dubbo服务,所以需要获取引用的field/method,以及具体引用的服务,dubbo内部已经做了这些工作,其值就是保存在AnnotatedMethodElement和AnnotatedFieldElement中,建议看一下代码,dubbo源码中将AnnotatedFieldElement设置为public,而AnnotatedMethodElement却设置成private,导致AnnotatedMethodElement根本无法在外部使用,AnnotatedFieldElement却可以在外部使用。咱们抛开编程知识,就从对称性来说这都是不合理的。

2 建议仔细看一下代码,DubboConfigBindingBeanPostProcessor 中的get/set根据就在内部使用,哪么什么属性可以变更
private boolean ignoreUnknownFields = true;
private boolean ignoreInvalidFields = true;

@htynkn
Copy link
Member

htynkn commented Sep 4, 2019

@mercyblitz 帮忙看下这个PR吧。
另外PR合并的是2.6.x,看下有无pick到2.7.x的必要

@apache apache deleted a comment from CLAassistant Sep 8, 2019
@CLAassistant
Copy link

CLAassistant commented Sep 9, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
6 out of 19 committers have signed the CLA.

✅ beiwei30
✅ mercyblitz
✅ qixiaobo
✅ chickenlj
✅ brucelwl
✅ cyejing
❌ kexianjun
❌ dongYES
❌ uglycow
❌ LiZhenNet
❌ lexburner
❌ CrazyHZM
❌ kuaike
❌ P3trur0
❌ nzomkxia
❌ wanghbxxxx
❌ cvictory
❌ biyuhao
❌ 356082462
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@beiwei30 beiwei30 left a comment

Choose a reason for hiding this comment

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

LGTM, but I will ask Mercy Ma to double check.

@mercyblitz mercyblitz merged commit 116c9c3 into apache:2.6.x Sep 10, 2019
This was referenced Sep 15, 2019
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.