添加了一个新的注解EnableAutoResfresh #888
Conversation
Codecov Report
@@ Coverage Diff @@
## master #888 +/- ##
============================================
- Coverage 46.76% 46.65% -0.11%
- Complexity 1540 1545 +5
============================================
Files 352 352
Lines 9717 9767 +50
Branches 963 966 +3
============================================
+ Hits 4544 4557 +13
- Misses 4831 4866 +35
- Partials 342 344 +2
Continue to review full report at Codecov.
|
|
干的事情就是添加了一个名为EnableAutoResfresh 的注解 |
|
Changes Unknown when pulling e464ac8 on tonyDJB:master into ** on ctripcorp:master**. |
|
好的，这几天比较忙，周末看下。 |
There was a problem hiding this comment.
Thanks for the pull requests. Overall, it looks good.
Please check the comments.
Besides, there is a similar pr #898, which will update the value by default(no EnableAutoResfresh).
I'm not sure if we should change the value directly or allow the user to customize (in application level, or in method/field level)
| processMethods(bean, clazz.getDeclaredMethods()); | ||
| return bean; | ||
| } | ||
| @Override |
There was a problem hiding this comment.
Please use Apollo's code style so that we won't have code style differences.
There was a problem hiding this comment.
Thanks for your comment, I will reformat my code.
| ReflectionUtils.makeAccessible(field); | ||
| //If it is the processing of complex data types, I suggest not to do AutoRefresh here or to handle it yourself | ||
| //add by 258737400@qq.com 2017-12-18 | ||
| String type = field.getGenericType().toString(); |
There was a problem hiding this comment.
Remember we should always use Spring's environment.getProperty() to get the property value because the key might be overridden in a different hierarchy.
There was a problem hiding this comment.
What do you mean by modifying the code here?
| /** | ||
| * @author tony Jiang(258737400@qq.com) | ||
| */ | ||
| public interface AutoConfigChangeListener { |
There was a problem hiding this comment.
I don't think we need a new list of AutoConfigChangeListeners. If we want to make sure some listener is executed first, we could create a a new interface such as PriorityOrderedConfigChangeListener that extends ConfigChangeListener and allow the user to provide its order?
There was a problem hiding this comment.
My idea is to provide a flexible refresh mode for users:
-
based on ApolloConfigChangeListener annotation, let users write their own refresh code
-
based on AutoConfigChangeListener annotation, automatic assembly of data for simple data types
In addition, with regard to #898, I do not agree with this coercion, which forces all configuration variables to be refreshed automatically, thus losing the flexibility of the user.
Above all, I think it's better to provide an independent annotation, because this is the two separate listener behavior, and there is no distinction between execution sequence.
| /** | ||
| * Apollo namespace for the config, if not specified then default to application | ||
| */ | ||
| String value() default ConfigConsts.NAMESPACE_APPLICATION; |
There was a problem hiding this comment.
I think we don't require the use to specify the namespace, since spring property source has hierarchies, which means one key could exist in different namespaces.
There was a problem hiding this comment.
Thank you for your comment.
If namespace is not named, does the key of the same name in different namespace be confused with each other？
| /** | ||
| * @author tony jiang(258737400@qq.com) | ||
| */ | ||
| public class AutoRefreshDemo { |
There was a problem hiding this comment.
This change impacts a lot, so we should have more tests to cover the value change scenarios.
There was a problem hiding this comment.
Thank you for your comment.
I'm going to improve the test code.
|
Thank you for your advice in your busy schedule I submitted comment under your question. |
2018-1-9 从origin更新代码
这个注解的作用是在变量上声明需要框架自动刷新，这样就不需要自己再写onChange事件，对简单拆箱的变量能省不少事