-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
bugfix: configuration cache get value cast exception. #3293
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3293 +/- ##
=============================================
+ Coverage 51.26% 51.45% +0.19%
- Complexity 3318 3329 +11
=============================================
Files 616 616
Lines 20174 20206 +32
Branches 2517 2532 +15
=============================================
+ Hits 10342 10398 +56
+ Misses 8795 8766 -29
- Partials 1037 1042 +5
|
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Outdated
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Outdated
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationCache.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
把原有的 缓存Object 改成了缓存ObjectWrapper, 我认为这是给缓存这个功能带来了额外的复杂度。 //缓存结果为String
private static final ConcurrentHashMap<String, String> CONFIG_CACHE = new ConcurrentHashMap<>();
public Configuration proxy(Configuration originalConfiguration) {
// 之前创建的是增强类, 改为自定义包装类
return new ConfigurationDecorate(originalConfiguration) {
@Override
public String getLatestConfig(String dataId, String defaultValue, long timeoutMills) {
String cacheValue = CONFIG_CACHE.get(dataId);
if (null == cacheValue) {
cacheValue = super.getLatestConfig(dataId, defaultValue, timeoutMills);
CONFIG_CACHE.put(dataId, cacheValue);
}
return cacheValue;
}
};
}
// 自定义包装类的实现分为两组, get开头需要调用getLatestConfig的所有方法都不代理,直接调用父类的
// 其他的方法由委托执行
public abstract class ConfigurationDecorate extends AbstractConfiguration implements Configuration
//....
private Configuration delegate;
}
// 当然还有这次改进的事件通知的地方同样也要做改一下
@Override
public void onChangeEvent(ConfigurationChangeEvent event) {
// The oldValue.data only exists in the cache when it is not null.
String newValue = event.getNewValue();
if (StringUtils.isNotBlank(newValue)) {
String oldValue = CONFIG_CACHE.get(event.getDataId());
if (!Objects.equals(oldValue, newValue)) {
CONFIG_CACHE.put(event.getDataId(), newValue);
}
} else {
CONFIG_CACHE.remove(event.getDataId());
}
} |
一种新的思路,[赞]。不过个人觉得缓存不应该只存单一的string类型,这样会导致调用者调用麻烦(需要类型转换)。 关于类型转换,这里其实有参考一些开源缓存的设计规范(JSR107): 最终个人觉得这里使用开源缓存框架实现比较好。 |
@ls9527 类型转换的设计还有一个用意就是不让调用者转换。 |
@jsbxyyx 你说的很对, 这可能就是最初设计缓存的想法吧。 |
首先你这个思路一开始我们是想到过,只代理latestconfig,一开始函数也不叫这个名,而且所有个getxxxx最后其实都是调的latestconfig,但是因为代理了latestconfig的话,会导致file作为配置中心的实现,FileConfiguration中的监听功能收到影响,因为监听的实现就是死循环一秒读一次文件,看看有没有变化,读取到对应值,而因为你代理了latestconfig,会导致这个file作为配置中心永远的去缓存里读配置,也就是失效了,而且spring boot的file是被SpringBootConfigurationProvider代理了一层,这是个棘手的问题,不知道仁兄是否有什么解决方案,能兼容所有的配置中心。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ⅰ. Describe what this PR did
bugfix: fix configuration cache value change to new value is string, when get boolean then throws cast exception.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews