dubbo icon indicating copy to clipboard operation
dubbo copied to clipboard

remind user to add qos dependency.(Fixes #10682)

Open pandaapo opened this issue 3 years ago • 2 comments

What is the purpose of the change

Fixes #10682

Brief changelog

Although configure dubbo.application.qos-enable=true, it may not work. You may find provider is started successfully without qos server. In fact, qos dependency is necessary for qos server to start successfully. But the reason is not obvious. ~~So I define an annotation @QosServer. It has no function, but can guide developers to import qos dependency.~~ I check qos-enable during app startup. When there is no qos-enable=false and no dubbo-qos dependency, logg.warn some information.

Verifying this change

run dubbo-demo-xml-provider

Checklist

  • [x] Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • [x] Each commit in the pull request should have a meaningful subject line and body.
  • [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [ ] Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • [ ] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • [ ] Add some description to dubbo-website project if you are requesting to add a feature.
  • [ ] GitHub Actions works fine on your own branch.
  • [ ] If this contribution is large, please follow the Software Donation Guide.

pandaapo avatar Sep 27 '22 04:09 pandaapo

Codecov Report

Merging #10683 (9c1d978) into 3.1 (199b059) will decrease coverage by 0.68%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                3.1   #10683      +/-   ##
============================================
- Coverage     65.23%   64.54%   -0.69%     
+ Complexity      469      467       -2     
============================================
  Files          1336     1336              
  Lines         56932    56968      +36     
  Branches       8437     8400      -37     
============================================
- Hits          37140    36771     -369     
- Misses        15821    16241     +420     
+ Partials       3971     3956      -15     
Impacted Files Coverage Δ
...org/apache/dubbo/config/context/ConfigManager.java 85.26% <100.00%> (+0.99%) :arrow_up:
...ache/dubbo/config/utils/ConfigValidationUtils.java 78.40% <100.00%> (+1.01%) :arrow_up:
...ubboDefaultPropertiesEnvironmentPostProcessor.java 86.84% <100.00%> (ø)
...luster/router/script/ScriptStateRouterFactory.java 0.00% <0.00%> (-100.00%) :arrow_down:
...zookeeper/curator/CuratorZookeeperTransporter.java 0.00% <0.00%> (-100.00%) :arrow_down:
...pc/cluster/router/file/FileStateRouterFactory.java 0.00% <0.00%> (-80.96%) :arrow_down:
...o/rpc/cluster/router/script/ScriptStateRouter.java 0.00% <0.00%> (-67.31%) :arrow_down:
...mmon/serialize/fastjson2/FastJson2ObjectInput.java 0.00% <0.00%> (-62.50%) :arrow_down:
...mon/serialize/fastjson2/FastJson2ObjectOutput.java 0.00% <0.00%> (-56.25%) :arrow_down:
...ting/zookeeper/curator/CuratorZookeeperClient.java 15.38% <0.00%> (-55.99%) :arrow_down:
... and 93 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Sep 27 '22 05:09 codecov-commenter

please fix uts

AlbumenJ avatar Oct 11 '22 02:10 AlbumenJ

image

AlbumenJ avatar Oct 17 '22 03:10 AlbumenJ

image

I want to test ConfigValidationUtils#checkQosDependency() even though it is private, but the method has no testable item except logger#warn(). So I intend to mock logger field, but it's a final field. I have to handle it with reflection. Now JDK17 seems forbid this operation for safety. I have no another good idea for now. Should I just test whether this method can be called by #validateApplicationConfig() correctly, not the inner logic? Could you give me some better suggestions?

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),但是该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

pandaapo avatar Oct 17 '22 05:10 pandaapo

image

I want to test ConfigValidationUtils#checkQosDependency() even though it is private, but the method has no testable item except logger#warn(). So I intend to mock logger field, but it's a final field. I have to handle it with reflection. Now JDK17 seems forbid this operation for safety. I have no another good idea for now. Should I just test whether this method can be called by #validateApplicationConfig() correctly, not the inner logic? Could you give me some better suggestions?

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),但是该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

junit 可以配置验证的版本,配置 range 到 jdk 14 就行

AlbumenJ avatar Oct 18 '22 01:10 AlbumenJ

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

junit 可以配置验证的版本,配置 range 到 jdk 14 就行

逐版验证了,从jdk 12开始就不行。

pandaapo avatar Oct 18 '22 09:10 pandaapo

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

junit 可以配置验证的版本,配置 range 到 jdk 14 就行

逐版验证了,从jdk 12开始就不行。

是不是 mockstatic 版本太低了

AlbumenJ avatar Oct 19 '22 02:10 AlbumenJ

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

junit 可以配置验证的版本,配置 range 到 jdk 14 就行

逐版验证了,从jdk 12开始就不行。

是不是 mockstatic 版本太低了

mockstatic 没问题,ConfigValidationUtils中公有的静态方法能正常mock(当然私有方法不行,官方好像也不打算支持)。 主要是ConfigValidationUtils中的logger是final的,Mock该属性以后没办法注入进去(即使最新的Mockito也不支持),只好利用反射去掉ConfigValidationUtils的Mock类中该字段的final修饰符,但是jdk>=12不支持。

目前能想到的解决办法有以下4种: 1、就是当前PR的:用Field#getModifiers() & ~Modifier.FINAL的反射方式来处理final修饰符。(可以测试logger是否打印预期的日志)(适用jdk8~11) 2、直接把ConfigValidationUtils中logger的final修饰符去掉。(可以测试logger是否打印预期的日志)(适用jdk8~17) 3、把ConfigValidationUtils#checkQosDependency()改成公有方法,并抛出没有Qos依赖的异常。(可以测试是否抛出指定的异常)(适用jdk8~17) 4、用MethodHandles.privateLookupIn(Field.class, MethodHandles.lookup())的反射方式来处理final修饰符。(可以测试logger是否打印预期的日志)(适用jdk9~14)

pandaapo avatar Oct 21 '22 05:10 pandaapo

想测试ConfigValidationUtils#checkQosDependency()方法(虽然是私有方法),该方法没有返回值、没有抛异常之类的可测试项,只有logger打印日志。于是就考虑注入Mock(logger),但是logger是final字段,只好利用反射来处理下,但是JDK17好像为提高安全性不支持这种操作。一时间没想到好办法。 考虑放弃测试该方法内部,只测试该方法是否能被 #validateApplicationConfig()正确调用。能否提供一些其他解决思路?谢谢。

junit 可以配置验证的版本,配置 range 到 jdk 14 就行

逐版验证了,从jdk 12开始就不行。

是不是 mockstatic 版本太低了

mockstatic 没问题,ConfigValidationUtils中公有的静态方法能正常mock(当然私有方法不行,官方好像也不打算支持)。 主要是ConfigValidationUtils中的logger是final的,Mock该属性以后没办法注入进去(即使最新的Mockito也不支持),只好利用反射去掉ConfigValidationUtils的Mock类中该字段的final修饰符,但是jdk>=12不支持。

目前能想到的解决办法有以下4种: 1、就是当前PR的:用Field#getModifiers() & ~Modifier.FINAL的反射方式来处理final修饰符。(可以测试logger是否打印预期的日志)(适用jdk8~11) 2、直接把ConfigValidationUtils中logger的final修饰符去掉。(可以测试logger是否打印预期的日志)(适用jdk8~17) 3、把ConfigValidationUtils#checkQosDependency()改成公有方法,并抛出没有Qos依赖的异常。(可以测试是否抛出指定的异常)(适用jdk8~17) 4、用MethodHandles.privateLookupIn(Field.class, MethodHandles.lookup())的反射方式来处理final修饰符。(可以测试logger是否打印预期的日志)(适用jdk9~14)

用第 2 种吧,3 改成公共方法相对于对外开放了,这个 API 不需要向用户公开的

AlbumenJ avatar Oct 22 '22 08:10 AlbumenJ