[Optimization] Replace Timer to ScheduleExecutorService #4208

Oliverwqcwrw · 2022-04-26T00:41:30Z

Make sure set the target branch to develop

What is the purpose of the change

Replace Timer to ScheduleExecutorService

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

lwclover · 2022-04-26T01:02:09Z

Two small suggestions.

cserwen · 2022-04-26T02:04:51Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -317,7 +320,8 @@ class StatsBenchmarkBatchProducer {

    private final LongAdder sendMessageFailedCount = new LongAdder();

-    private final Timer timer = new Timer("BenchmarkTimerThread", true);
+    private final ScheduledExecutorService executorService = new ScheduledThreadPoolExecutor(1,
+            new BasicThreadFactory.Builder().namingPattern("BenchmarkTimerThread-%d").daemon(true).build());


Maybe it's better to use ThreadFactoryImpl, just like this:

rocketmq/broker/src/main/java/org/apache/rocketmq/broker/BrokerController.java

Line 136 in 5d01023

private final ScheduledExecutorService scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(new ThreadFactoryImpl(

Emmm, I prefer to like present way

cserwen · 2022-04-26T02:08:21Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -360,17 +364,17 @@ public LongAdder getSendMessageFailedCount() {

    public void start() {

-        timer.scheduleAtFixedRate(new TimerTask() {
+        executorService.scheduleAtFixedRate(new TimerTask() {


The code can be more concise, such as using a lambda expression

HScarb · 2022-04-26T02:33:52Z

The client module will be compile with jdk 1.7 or jdk 1.6, which not support lambda

lwclover · 2022-04-26T00:57:33Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -360,17 +364,17 @@ public LongAdder getSendMessageFailedCount() {

    public void start() {

-        timer.scheduleAtFixedRate(new TimerTask() {
+        executorService.scheduleAtFixedRate(new TimerTask() {


It seems better to implement Runnable anonymously

lwclover · 2022-04-26T00:58:19Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -317,7 +320,8 @@ class StatsBenchmarkBatchProducer {

    private final LongAdder sendMessageFailedCount = new LongAdder();

-    private final Timer timer = new Timer("BenchmarkTimerThread", true);
+    private final ScheduledExecutorService executorService = new ScheduledThreadPoolExecutor(1,
+            new BasicThreadFactory.Builder().namingPattern("BenchmarkTimerThread-%d").daemon(true).build());


A unified style is recommended

I don't get your meaning,Can you provide more detail info

same as @cserwen mentioned.

Ok,Thanks for your suggestions

Oliverwqcwrw · 2022-04-26T06:30:30Z

I have modified it,Please check if have other issue

cserwen · 2022-04-26T06:31:20Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -317,7 +318,8 @@ class StatsBenchmarkBatchProducer {

    private final LongAdder sendMessageFailedCount = new LongAdder();

-    private final Timer timer = new Timer("BenchmarkTimerThread", true);
+    private final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(new ThreadFactoryImpl(
+            "BenchmarkTimerThread"));


daemon thread

Thanks for your checking,I have solved it

codecov-commenter · 2022-04-26T09:11:24Z

Codecov Report

Merging #4208 (1a61d3c) into develop (5d01023) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #4208      +/-   ##
=============================================
- Coverage      48.13%   48.09%   -0.04%     
+ Complexity      5038     5030       -8     
=============================================
  Files            636      636              
  Lines          42506    42506              
  Branches        5568     5568              
=============================================
- Hits           20459    20443      -16     
- Misses         19564    19575      +11     
- Partials        2483     2488       +5

Impacted Files	Coverage Δ
...rg/apache/rocketmq/common/stats/StatsSnapshot.java	`84.61% <0.00%> (-15.39%)`	⬇️
.../apache/rocketmq/common/stats/MomentStatsItem.java	`38.09% <0.00%> (-9.53%)`	⬇️
...ache/rocketmq/common/stats/MomentStatsItemSet.java	`39.13% <0.00%> (-8.70%)`	⬇️
...org/apache/rocketmq/common/stats/StatsItemSet.java	`46.26% <0.00%> (-7.47%)`	⬇️
...va/org/apache/rocketmq/common/stats/StatsItem.java	`50.00% <0.00%> (-5.00%)`	⬇️
...ketmq/common/protocol/body/ConsumerConnection.java	`95.83% <0.00%> (-4.17%)`	⬇️
...rocketmq/common/message/MessageClientIDSetter.java	`73.80% <0.00%> (-1.20%)`	⬇️
...a/org/apache/rocketmq/store/StoreStatsService.java	`36.97% <0.00%> (-1.13%)`	⬇️
...ent/impl/consumer/DefaultLitePullConsumerImpl.java	`68.79% <0.00%> (-0.52%)`	⬇️
...mq/client/impl/producer/DefaultMQProducerImpl.java	`45.21% <0.00%> (-0.38%)`	⬇️
... and 10 more

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 5d01023...1a61d3c. Read the comment docs.

coveralls · 2022-04-26T09:12:33Z

Coverage increased (+0.07%) to 52.068% when pulling 1a61d3c on Oliverwqcwrw:develop-replace-timer-2 into 9de7be4 on apache:develop.

coveralls · 2022-04-26T09:12:34Z

Coverage increased (+0.07%) to 52.068% when pulling 1a61d3c on Oliverwqcwrw:develop-replace-timer-2 into 9de7be4 on apache:develop.

HScarb · 2022-04-26T16:48:43Z

example/src/main/java/org/apache/rocketmq/example/benchmark/BatchProducer.java

@@ -317,7 +318,8 @@ class StatsBenchmarkBatchProducer {

    private final LongAdder sendMessageFailedCount = new LongAdder();

-    private final Timer timer = new Timer("BenchmarkTimerThread", true);
+    private final ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(new ThreadFactoryImpl(
+            "BenchmarkTimerThread", Boolean.TRUE));


The indent here maybe wrong, pls import rmq_codeStyle.xml in style folder and reformat the code

AFAIK, if indent is wrong,the checking will not passing

* replace Timer to ScheduleExecutorService * replace timetask to runnable * Change the style of creating ThreadPoolExecutor * Remove unused import * set daemon * checkstyle

replace Timer to ScheduleExecutorService

24ffbbe

cserwen reviewed Apr 26, 2022

View reviewed changes

lwclover requested changes Apr 26, 2022

View reviewed changes

replace timetask to runnable

93b2be1

Oliverwqcwrw force-pushed the develop-replace-timer-2 branch from 48508df to 93b2be1 Compare April 26, 2022 04:01

Oliverwqcwrw added 2 commits April 26, 2022 13:59

Change the style of creating ThreadPoolExecutor

14e6908

Remove unused import

25b84a5

cserwen reviewed Apr 26, 2022

View reviewed changes

Oliverwqcwrw added 2 commits April 26, 2022 14:38

set daemon

57d9e92

checkstyle

1a61d3c

cserwen approved these changes Apr 26, 2022

View reviewed changes

HScarb approved these changes Apr 26, 2022

View reviewed changes

HScarb reviewed Apr 26, 2022

View reviewed changes

duhenglucky approved these changes Apr 27, 2022

View reviewed changes

duhenglucky merged commit a800706 into apache:develop Apr 27, 2022

duhenglucky added this to the 4.9.4 milestone Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Optimization] Replace Timer to ScheduleExecutorService #4208

[Optimization] Replace Timer to ScheduleExecutorService #4208

Oliverwqcwrw commented Apr 26, 2022

lwclover commented Apr 26, 2022

cserwen Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

cserwen Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

HScarb commented Apr 26, 2022

lwclover Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

lwclover Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

lwclover Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

Oliverwqcwrw commented Apr 26, 2022

cserwen Apr 26, 2022

Oliverwqcwrw Apr 26, 2022

codecov-commenter commented Apr 26, 2022 •

edited

Loading

coveralls commented Apr 26, 2022

coveralls commented Apr 26, 2022

HScarb Apr 26, 2022

Oliverwqcwrw Apr 27, 2022

[Optimization] Replace Timer to ScheduleExecutorService #4208

[Optimization] Replace Timer to ScheduleExecutorService #4208

Conversation

Oliverwqcwrw commented Apr 26, 2022

What is the purpose of the change

lwclover commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HScarb commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oliverwqcwrw commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 26, 2022 • edited Loading

Codecov Report

coveralls commented Apr 26, 2022

coveralls commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 26, 2022 •

edited

Loading