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 exceptions lead to increased CPU usage #2391

Merged
merged 10 commits into from
Mar 17, 2020

Conversation

funky-eyes
Copy link
Contributor

Ⅰ. Describe what this PR did

optimize file configuration reading

Ⅱ. Does this pull request fix one issue?

#2387

Ⅲ. 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 Mar 11, 2020

Codecov Report

Merging #2391 into develop will decrease coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2391      +/-   ##
=============================================
- Coverage      51.80%   51.80%   -0.01%     
  Complexity      2696     2696              
=============================================
  Files            517      517              
  Lines          16761    16764       +3     
  Branches        2028     2028              
=============================================
+ Hits            8683     8684       +1     
- Misses          7267     7269       +2     
  Partials         811      811              
Impacted Files Coverage Δ Complexity Δ
...c/main/java/io/seata/config/FileConfiguration.java 36.43% <20.00%> (-0.08%) 6.00 <0.00> (ø)

@@ -306,7 +308,9 @@ public FileListener(String dataId, ConfigurationChangeListener listener) {

@Override
public void onChangeEvent(ConfigurationChangeEvent event) {
while (true) {
ScheduledExecutorService onChangeEventExecutor =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ScheduledExecutorService would be instantiate more time if I listen different config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL @l81893521

@threefish
Copy link
Contributor

测试当前PR分支

结果

复现错误,CPU性能正常。理论上在生产环境是不会产生该异常。

复现错误

当前classLoader: RestartClassLoader
启动服务后,修改controller类方法代码,触发devtool restart。
devtool restart 后每隔1秒输出一次 异常信息。
服务无法正常访问。

异常信息

2020-03-11 21:23:59.362 INFO com.alibaba.nacos.client.naming Line:195 - current ips:(0) service: DEFAULT_GROUP@@serverAddr -> [] 2020-03-11 21:23:59.362 INFO com.alibaba.nacos.client.naming Line:195 - current ips:(0) service: DEFAULT_GROUP@@flow-center-business-demo-client -> [] 2020-03-11 21:23:59.964 ERROR io.seata.config.FileConfiguration Line:323 - fileListener execute error:org.springframework.boot.web.servlet.context.AnnotationConfigServletWebServerApplicationContext@af3a716 has been closed already java.lang.IllegalStateException: org.springframework.boot.web.servlet.context.AnnotationConfigServletWebServerApplicationContext@af3a716 has been closed already at org.springframework.context.support.AbstractApplicationContext.assertBeanFactoryActive(AbstractApplicationContext.java:1092) at org.springframework.context.support.AbstractApplicationContext.getBean(AbstractApplicationContext.java:1125) at io.seata.spring.boot.autoconfigure.provider.SpringBootConfigurationProvider.get(SpringBootConfigurationProvider.java:93) at io.seata.spring.boot.autoconfigure.provider.SpringBootConfigurationProvider.access$100(SpringBootConfigurationProvider.java:43) at io.seata.spring.boot.autoconfigure.provider.SpringBootConfigurationProvider$1.intercept(SpringBootConfigurationProvider.java:56) at io.seata.config.FileConfiguration$$EnhancerByCGLIB$$862af1eb.getConfig(<generated>) at io.seata.config.FileConfiguration$FileListener.lambda$onChangeEvent$0(FileConfiguration.java:315) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) at java.util.concurrent.FutureTask.runAndReset$$$capture(FutureTask.java:308) at java.util.concurrent.FutureTask.runAndReset(FutureTask.java) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.lang.Thread.run(Thread.java:748)

@funky-eyes
Copy link
Contributor Author

@threefish how long will it not output an exception?or is it always exist?

@zjinlei zjinlei added this to the 1.2.0 milestone Mar 12, 2020
try {
Thread.sleep(LISTENER_CONFIG_INTERNAL);
} catch (InterruptedException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

use

LOGGER.error

instead of

e.printStackTrace();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use

LOGGER.error

instead of

e.printStackTrace();

ok

LOGGER.error("fileListener execute error:{}", exx.getMessage());
}
try {
Thread.sleep(LISTENER_CONFIG_INTERNAL);
Copy link
Member

Choose a reason for hiding this comment

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

Please help correct the spelling by the way.Thank you!
LISTENER_CONFIG_INTERNAL->LISTENER_CONFIG_INTERVAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LISTENER_CONFIG_INTERVAL

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

cpu has behaved normally。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cpu has behaved normally。

If there are no questions, please reply to PTAL separately

@threefish
Copy link
Contributor

cpu has behaved normally。thanks

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zjinlei zjinlei 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 changed the title optimize:file configuration reading bugfix: configuration exceptions lead to increased CPU usage Mar 17, 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 bd61afe into apache:develop Mar 17, 2020
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.

None yet

7 participants