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

add apollo datasource extension #46

Merged
merged 1 commit into from
Aug 8, 2018
Merged

Conversation

nobodyiam
Copy link
Contributor

@nobodyiam nobodyiam commented Aug 8, 2018

Describe what this PR does / why we need it

Add Apollo datasource extension.

Does this pull request fix one issue?

Closes #42

Describe how you did it

See ApolloDataSource.java

Describe how to verify it

See ApolloDataSourceDemo.java

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #46 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #46      +/-   ##
============================================
- Coverage     45.86%   45.68%   -0.19%     
+ Complexity      552      550       -2     
============================================
  Files           113      113              
  Lines          3800     3800              
  Branches        533      533              
============================================
- Hits           1743     1736       -7     
- Misses         1846     1851       +5     
- Partials        211      213       +2
Impacted Files Coverage Δ Complexity Δ
...ntinel/slots/statistic/metric/WindowLeapArray.java 70.96% <0%> (-9.68%) 7% <0%> (-1%)
...ibaba/csp/sentinel/eagleeye/EagleEyeLogDaemon.java 24.24% <0%> (-6.07%) 5% <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 73b0979...c44081b. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Aug 8, 2018
@sczyh30 sczyh30 modified the milestone: 0.1.1 Aug 8, 2018
//change is never null because the listener will only notify for this key
if (change != null) {
RecordLog.info(
String.format("[ApolloDataSource] Received config changes: %s", changeEvent.getChange(flowRulesKey)));
Copy link
Member

Choose a reason for hiding this comment

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

changeEvent.getChange(flowRulesKey) 与前面重复,可替换为 change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里重构的时候疏忽了,后续改一下

RecordLog.info(
String.format("[ApolloDataSource] Received config changes: %s", changeEvent.getChange(flowRulesKey)));
}
loadAndUpdateRules();
Copy link
Member

Choose a reason for hiding this comment

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

这个地方有一个疑问:我看了一下 ConfigChange 的定义,貌似可以直接从中获取 newValue,然后进行更新;而 loadAndUpdateRules 方法中调用了 loadConfig 方法,后者实际调用 readSource 方法主动读取数据,这会不会导致多读一次?

Copy link
Contributor Author

@nobodyiam nobodyiam Aug 8, 2018

Choose a reason for hiding this comment

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

ConfigChange中确实是可以拿到newValue的,不过ConfigChange有多种change type:ADDED, MODIFIED和DELETED。

对DELETED类型,如果直接用newValue其实是空的,所以再走一次loadAndUpdateRules可以确保默认值会被用到。

对于Apollo而言,只有ConfigService.getConfig会触发网络调用,对于config.getProperty始终是从内存中读取配置的,所以从内存中再读取一次也是没有性能影响的,加之上述的defaultValue逻辑,所以选择了复用loadAndUpdateRules。

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit b51c3ad into alibaba:master Aug 8, 2018
@sczyh30
Copy link
Member

sczyh30 commented Aug 8, 2018

Thanks for your contribution! 👍

@sczyh30 sczyh30 removed the to-review To review label Aug 8, 2018
sczyh30 pushed a commit that referenced this pull request Aug 8, 2018
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 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.

[Feature] DataSource integration for Apollo
4 participants