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

bugfix: configuration cache get value cast exception. #3293

Merged
merged 16 commits into from
Dec 16, 2020
Merged

bugfix: configuration cache get value cast exception. #3293

merged 16 commits into from
Dec 16, 2020

Conversation

jsbxyyx
Copy link
Member

@jsbxyyx jsbxyyx commented Nov 20, 2020

Ⅰ. 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

@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #3293 (1893820) into develop (aeeda42) will increase coverage by 0.19%.
The diff coverage is 78.04%.

Impacted file tree graph

@@              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     
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/seata/config/ConfigurationCache.java 66.66% <78.04%> (+21.66%) 11.00 <0.00> (+1.00)
...java/io/seata/config/ConfigurationChangeEvent.java 76.92% <0.00%> (+38.46%) 9.00% <0.00%> (+3.00%)
...in/java/io/seata/config/AbstractConfiguration.java 71.42% <0.00%> (+42.85%) 15.00% <0.00%> (+8.00%)
.../java/io/seata/config/ConfigurationChangeType.java 100.00% <0.00%> (+100.00%) 1.00% <0.00%> (+1.00%)

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM

@funky-eyes funky-eyes added the module/config config module label Dec 1, 2020
@ls9527
Copy link
Contributor

ls9527 commented Dec 5, 2020

把原有的 缓存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());
        }
    }

@jsbxyyx
Copy link
Member Author

jsbxyyx commented Dec 7, 2020

把原有的 缓存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):
缓存在设计上是分为两种的,引用存储和值存储。
这里采用的是值存储,那么就会涉及到存储时的序列化,反序列化。代码中的类型转换实际上充当反序列化。(但是感觉有点四不像)

最终个人觉得这里使用开源缓存框架实现比较好。

@jsbxyyx
Copy link
Member Author

jsbxyyx commented Dec 7, 2020

@ls9527 类型转换的设计还有一个用意就是不让调用者转换。

@ls9527
Copy link
Contributor

ls9527 commented Dec 7, 2020

@jsbxyyx 你说的很对, 这可能就是最初设计缓存的想法吧。
直接缓存用户需要最终的值。 而我这里提议的,缓存的值会经过一次转换。
要是按照这个思路做个二级缓存你觉得怎么样? 我觉得有点麻烦了,不过也算符合你说的JSR规范吧
一个缓存string, 一个缓存object

@funky-eyes
Copy link
Contributor

@jsbxyyx 你说的很对, 这可能就是最初设计缓存的想法吧。
直接缓存用户需要最终的值。 而我这里提议的,缓存的值会经过一次转换。
要是按照这个思路做个二级缓存你觉得怎么样? 我觉得有点麻烦了,不过也算符合你说的JSR规范吧
一个缓存string, 一个缓存object

首先你这个思路一开始我们是想到过,只代理latestconfig,一开始函数也不叫这个名,而且所有个getxxxx最后其实都是调的latestconfig,但是因为代理了latestconfig的话,会导致file作为配置中心的实现,FileConfiguration中的监听功能收到影响,因为监听的实现就是死循环一秒读一次文件,看看有没有变化,读取到对应值,而因为你代理了latestconfig,会导致这个file作为配置中心永远的去缓存里读配置,也就是失效了,而且spring boot的file是被SpringBootConfigurationProvider代理了一层,这是个棘手的问题,不知道仁兄是否有什么解决方案,能兼容所有的配置中心。

@funky-eyes funky-eyes added this to the 1.5.0 milestone Dec 10, 2020
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly slievrly merged commit 6075716 into apache:develop Dec 16, 2020
@jsbxyyx jsbxyyx deleted the configuration_cache branch December 17, 2020 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/config config module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants