iframe-proxy

smallzhongfeng · 2022-08-03T10:11:14Z

What changes were proposed in this pull request?

When the main thread of the shuffleServer exits, the heartbeat thread should exit as well.

Why are the changes needed?

More formal.

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need.

smallzhongfeng · 2022-08-03T10:14:11Z

codecov-commenter · 2022-08-03T10:47:05Z

Codecov Report

Merging #121 (4cd39ca) into master (fce4bb6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #121      +/-   ##
============================================
- Coverage     57.17%   57.16%   -0.01%     
+ Complexity     1201     1200       -1     
============================================
  Files           150      150              
  Lines          8177     8178       +1     
  Branches        773      773              
============================================
  Hits           4675     4675              
  Misses         3257     3257              
- Partials        245      246       +1

Impacted Files	Coverage Δ
...ava/org/apache/uniffle/common/web/JettyServer.java	`56.25% <100.00%> (-0.68%)`	⬇️
...a/org/apache/uniffle/server/RegisterHeartBeat.java	`44.64% <100.00%> (+2.05%)`	⬆️
...storage/handler/impl/DataSkippableReadHandler.java	`81.25% <0.00%> (-3.13%)`	⬇️
.../apache/uniffle/coordinator/ClientConfManager.java	`91.54% <0.00%> (-1.41%)`	⬇️
...va/org/apache/uniffle/common/util/ThreadUtils.java	`50.00% <0.00%> (+50.00%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

roryqi · 2022-08-03T11:03:17Z

Why do we need this patch?

smallzhongfeng · 2022-08-03T11:51:32Z

I'm not sure whether the main thread of the shuffleServer will exit unexpectedly, but is it meaningful to set the heartbeat scheduling thread as the daemon thread.

roryqi · 2022-08-03T11:54:49Z

I'm not sure whether the main thread of the shuffleServer will exit unexpectedly, but is it meaningful to set the heartbeat scheduling thread as the daemon thread.

Ok, have you checked similar problems in other code in our project? Could you fix them together?

smallzhongfeng · 2022-08-03T12:06:38Z

The same as ExecutorService executor = Executors.newSingleThreadExecutor(); I think this one needs to be set to a daemon thread as well. What do you think?@jerqi

roryqi · 2022-08-03T12:09:58Z

The same as ExecutorService executor = Executors.newSingleThreadExecutor(); I think this one needs to be set to a daemon thread as well. What do you think?@jerqi

I think we should check all types ExecutorService. A similar pr ba47aa0

smallzhongfeng · 2022-08-03T12:43:45Z

Reference pr you metioned, I set all unnecessary user threads as daemon.

roryqi · 2022-08-03T12:45:42Z


  protected void sendCommit() {
-    ExecutorService executor = Executors.newSingleThreadExecutor();
+    ExecutorService executor = Executors.newSingleThreadExecutor(ThreadUtils.getThreadFactory("bufferManagerSend-%d"));


This is a temporary thread pool. I think it's unnecessary to be a daemon thread pool.WDYT?

Yes, I think I made a mistake here, but I think a better way would have been to generate this executor at this object initialization. @jerqi

MEMORY_LOCALFILE and MEMROY_HDFS don't call commit operation, so we use a temporary thread pool.

roryqi · 2022-08-03T12:47:04Z

  @VisibleForTesting
  protected void sendCommit() {
-    ExecutorService executor = Executors.newSingleThreadExecutor();
+    ExecutorService executor = Executors.newSingleThreadExecutor(ThreadUtils.getThreadFactory("sendCommit-%d"));


This is a temporary thread pool. I think it's unnecessary to be a daemon thread pool.WDYT?

I agree with you here, there really is no need for a daemon thread.

roryqi · 2022-08-03T12:47:26Z

  @Test
  public void testServerMetricsConcurrently() throws Exception {
-    ExecutorService executorService = Executors.newFixedThreadPool(3);
+    ExecutorService executorService = Executors.newCachedThreadPool(


This is a temporary thread pool. I think it's unnecessary to be a daemon thread pool.WDYT?

roryqi · 2022-08-03T12:47:47Z

    CountDownLatch countDownLatch = new CountDownLatch(storageBasePaths.length);
    AtomicInteger successCount = new AtomicInteger();
-    ExecutorService executorService = Executors.newCachedThreadPool();
+    ExecutorService executorService = Executors.newCachedThreadPool(


This is a temporary thread pool. I think it's unnecessary to be a daemon thread pool.WDYT?

Would it be better to create the thread pool when the LocalStorageManager is initialized.

We only use this thread pool to delete the files once when the server start. I think it's no need.

roryqi · 2022-08-03T13:51:15Z

  private final LocalStorageChecker checker;
  private List<LocalStorage> unCorruptedStorages = Lists.newArrayList();
  private final Set<String> corruptedStorages = Sets.newConcurrentHashSet();
+  private final ExecutorService executorService = Executors.newCachedThreadPool(


We only use this thread pool to delete the files once when the server start. I think it's no need.

roryqi · 2022-08-03T14:03:07Z

  @Test
  public void testServerMetricsConcurrently() throws Exception {
-    ExecutorService executorService = Executors.newFixedThreadPool(3);
+    ExecutorService executorService = Executors.newCachedThreadPool();


Why do we modify this?

IIRC, a cachedThreadPool should be faster if the number of threads started is known though this is just a test class.

I'm not sure why these thread numbers are needed here.

We hope use multi-thread to call some methods, usually thread pool has specific number threads, more tasks is summited, more threads will be created util the thread number reach the max value. I guess that we just ensure multi-thread call the methods, I would like not to modify here.

roryqi

LGTM, thanks @smallzhongfeng

[Improvement] Set heartBeatExecutorService as daemon thread

c132cda

smallzhongfeng closed this Aug 3, 2022

smallzhongfeng reopened this Aug 3, 2022

smallzhongfeng closed this Aug 3, 2022

smallzhongfeng reopened this Aug 3, 2022

set other thread as daemon thread

10a5121

roryqi reviewed Aug 3, 2022

View reviewed changes

smallzhongfeng added 2 commits August 3, 2022 21:24

make temp thread pool return non-daemon thread

5758725

remove sth

1f477a9

smallzhongfeng requested a review from roryqi August 3, 2022 13:26

roryqi reviewed Aug 3, 2022

View reviewed changes

smallzhongfeng added 2 commits August 3, 2022 21:58

revert sth

c0fcefb

style pass

0f9ea10

roryqi reviewed Aug 3, 2022

View reviewed changes

revert sth

4cd39ca

roryqi approved these changes Aug 3, 2022

View reviewed changes

roryqi merged commit fd8ccdd into apache:master Aug 3, 2022

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

smallzhongfeng commented Aug 3, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

smallzhongfeng commented Aug 3, 2022

Uh oh!

codecov-commenter commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

roryqi commented Aug 3, 2022

Uh oh!

smallzhongfeng commented Aug 3, 2022

Uh oh!

roryqi commented Aug 3, 2022

Uh oh!

smallzhongfeng commented Aug 3, 2022

Uh oh!

roryqi commented Aug 3, 2022

Uh oh!

smallzhongfeng commented Aug 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallzhongfeng Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryqi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 3, 2022 •

edited

Loading

smallzhongfeng Aug 3, 2022 •

edited

Loading