[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method #1811

zwangsheng · 2023-08-14T07:06:51Z

What changes were proposed in this pull request?

Tweak the logic of MemoryManager#currentServingState

Add Unit Test for this function

graph TB

A(Check Used Memory) --> B{Reach Pause Replicate Threshold}
B --> | N | C{Reach Pause Push Threshold}
B --> | Y | Z(Trigger Pause Push and Replicate)
C --> | N | D{Reach Resume Threshold}
C --> | Y | Y(Trigger Pause Push but Resume Replicate)
D --> | N | E{In Pause Mode}
D --> | Y | X(Trigger Resume Push and Replicate)
E --> | N | U(Do Nothing)
E --> | Y | Y

Why are the changes needed?

Make this method logical, and add unit test to ensure logic won't be accidental modification

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add Unit Test

pan3793 · 2023-08-14T07:21:59Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

-                    memoryPressureListener ->
-                        memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
-                trimAllListeners();
+    // disabled trim thread in test case


this is hacky, and why?

During the unit test, we want to trigger the threshold for each state, in order to avoid this thread, reset the current service state.

For example, we set sortMemory to trigger PAUSE_PUSH state, we don't want this thread trim memory.

I don't think it makes much sense. Instead, memoryPressureListeners may be a good joint.

In the test scenario, memoryPressureListeners shall be empty. The trim won't do anything.

pan3793 · 2023-08-14T07:24:18Z

Make this method logical ...

Any details? What problem do you find in the current implementation?

zwangsheng · 2023-08-14T07:41:39Z

Make this method logical ...

Any details? What problem do you find in the current implementation?

The main thing is to add unit tests to ensure that this piece of logic is changed by mistake.

The modification of the method is to reduce judgments and avoid making unnecessary comparisons before the method returns.

zwangsheng · 2023-08-14T07:55:48Z

As sbt unit test fail, WIP @cfmcgrady

cfmcgrady · 2023-08-14T08:02:58Z

As sbt unit test fail, WIP @cfmcgrady

It occurred randomly, and I'm still investigating. Let's temporarily disregard it.

cfmcgrady · 2023-08-16T03:19:42Z

As sbt unit test fail, WIP @cfmcgrady

Fixed in #1818. cc @zwangsheng

zwangsheng · 2023-08-16T03:29:23Z

As sbt unit test fail, WIP @cfmcgrady

Fixed in #1818. cc @zwangsheng

Thanks, i'll rebase and retest.

…ryManager#currentServingState method

FMX · 2023-08-17T03:08:08Z

worker/src/test/scala/org/apache/celeborn/service/deploy/memory/MemoryManagerSuite.scala

@@ -42,4 +41,42 @@ class MemoryManagerSuite extends AnyFunSuite with BeforeAndAfterEach {
      caught.getMessage == s"Invalid config, ${WORKER_DIRECT_MEMORY_RATIO_PAUSE_REPLICATE.key}(0.85) " +
        s"should be greater than ${WORKER_DIRECT_MEMORY_RATIO_PAUSE_RECEIVE.key}(0.95)")
  }
+
+  test("[CELEBORN-888] Test MemoryManager#currentServingState trigger case") {


This test case looks good to me.

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

codecov · 2023-08-22T06:35:39Z

Codecov Report

Merging #1811 (7a517ce) into main (4f47d0a) will increase coverage by 0.02%.
Report is 5 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1811      +/-   ##
==========================================
+ Coverage   46.43%   46.45%   +0.02%     
==========================================
  Files         163      163              
  Lines       10135    10135              
  Branches      934      934              
==========================================
+ Hits         4705     4707       +2     
+ Misses       5118     5117       -1     
+ Partials      312      311       -1

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

FMX · 2023-08-22T08:18:29Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

-                        memoryPressureListener.onPause(TransportModuleConstants.REPLICATE_MODULE));
-                trimAllListeners();
+    // disabled trim thread in test case
+    if (!Boolean.valueOf(conf.get("celeborn.testing", "false"))) {


I see that in your test scenario the memory pressure listeners are empty so maybe this check is unnecessary?

Never thought this! Thanks for reminding, i will make changes.

FMX · 2023-08-22T08:23:18Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

-    }
-    if (pausePushData && pauseReplicate) {
+    // trigger pause both push and replicate
+    if (memoryUsage > pauseReplicateThreshold) {


Let's assume that a newbie comes and try to understand the method. Why should this method return PUSH_AND_REPLICATE_PAUSED when memory usage is above pauseReplicateThreshold? Will renaming pauseReplicateThreshold to pausePushAndReplicateThreshold be a better name for understanding?

Have same idea, maybe we should call conf celeborn.worker.directMemoryRatioToPauseReplicate => celeborn.worker.directMemoryRatioToPauseAll?

WDYT?

IMO we can add comment describing the logic so that we don't need to change the config name.

waitinfuture

LGTM, thanks! Merging to main/0.3

…oryManager#currentServingState method Tweak the logic of `MemoryManager#currentServingState` Add Unit Test for this function ```mermaid graph TB A(Check Used Memory) --> B{Reach Pause Replicate Threshold} B --> | N | C{Reach Pause Push Threshold} B --> | Y | Z(Trigger Pause Push and Replicate) C --> | N | D{Reach Resume Threshold} C --> | Y | Y(Trigger Pause Push but Resume Replicate) D --> | N | E{In Pause Mode} D --> | Y | X(Trigger Resume Push and Replicate) E --> | N | U(Do Nothing) E --> | Y | Y ``` Make this method logical, and add unit test to ensure logic won't be accidental modification No Add Unit Test Closes #1811 from zwangsheng/CELEBORN-888. Authored-by: zwangsheng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]>

cfmcgrady · 2023-08-23T10:07:01Z

@zwangsheng Could you please help by revising the state diagram docs/assets/img/backpressure.svg in line with this refactor? The current state diagram docs/assets/img/backpressure.svg is unclear.

zwangsheng · 2023-08-23T11:11:53Z

Could you please help by revising the state diagram docs/assets/img/backpressure.svg in line with this refactor? The current state diagram docs/assets/img/backpressure.svg is unclear.

Sure, i'd like to raise a pr to make those diagram clearly.

…se logic is reconstructed ### What changes were proposed in this pull request? Add a new `backpressure.svg` to replace the out-date one. ### Why are the changes needed? After #1811, we refactor celeborn worker back-pressure logic, we should add new flowchart for user to understand. ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? ![backpressure](https://github.com/apache/incubator-celeborn/assets/52876270/34f3f4b8-28cf-4cce-88a4-e6fee1886d94) Closes #1829 from zwangsheng/CELEBORN-905. Authored-by: zwangsheng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]>

…se logic is reconstructed Add a new `backpressure.svg` to replace the out-date one. After #1811, we refactor celeborn worker back-pressure logic, we should add new flowchart for user to understand. Yes ![backpressure](https://github.com/apache/incubator-celeborn/assets/52876270/34f3f4b8-28cf-4cce-88a4-e6fee1886d94) Closes #1829 from zwangsheng/CELEBORN-905. Authored-by: zwangsheng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]> (cherry picked from commit 2ffd6d7) Signed-off-by: zky.zhoukeyong <[email protected]>

…se logic is reconstructed Add a new `backpressure.svg` to replace the out-date one. After apache#1811, we refactor celeborn worker back-pressure logic, we should add new flowchart for user to understand. Yes ![backpressure](https://github.com/apache/incubator-celeborn/assets/52876270/34f3f4b8-28cf-4cce-88a4-e6fee1886d94) Closes apache#1829 from zwangsheng/CELEBORN-905. Authored-by: zwangsheng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]>

zwangsheng requested review from pan3793 and FMX and removed request for pan3793 August 14, 2023 07:08

pan3793 reviewed Aug 14, 2023

View reviewed changes

zwangsheng added 4 commits August 16, 2023 11:48

[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the Memo…

76f38af

…ryManager#currentServingState method

fix style

71f1580

try to fix sbt

2356a2d

try to fix sbt

8dd8a30

zwangsheng force-pushed the CELEBORN-888 branch from 57cad86 to 8dd8a30 Compare August 16, 2023 03:49

force to reach resume threshold

9c45387

zwangsheng requested a review from pan3793 August 16, 2023 07:09

FMX reviewed Aug 17, 2023

View reviewed changes

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java Show resolved Hide resolved

remove misleading comment

2c6f7e3

zwangsheng requested a review from FMX August 22, 2023 06:29

FMX reviewed Aug 22, 2023

View reviewed changes

fix comments

7a517ce

zwangsheng requested review from FMX and waitinfuture August 23, 2023 08:46

waitinfuture approved these changes Aug 23, 2023

View reviewed changes

waitinfuture closed this in 479510c Aug 23, 2023

zwangsheng mentioned this pull request Aug 23, 2023

[CELEBORN-905] Redraw the flowchart backpressure.svg after worker pause logic is reconstructed #1829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method #1811

[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method #1811

zwangsheng commented Aug 14, 2023 •

edited

Loading

pan3793 Aug 14, 2023

zwangsheng Aug 14, 2023

pan3793 Aug 14, 2023

FMX Aug 17, 2023

pan3793 commented Aug 14, 2023

zwangsheng commented Aug 14, 2023

zwangsheng commented Aug 14, 2023

cfmcgrady commented Aug 14, 2023

cfmcgrady commented Aug 16, 2023

zwangsheng commented Aug 16, 2023

FMX Aug 17, 2023

codecov bot commented Aug 22, 2023 •

edited

Loading

FMX Aug 22, 2023

zwangsheng Aug 23, 2023

FMX Aug 22, 2023

zwangsheng Aug 22, 2023 •

edited

Loading

waitinfuture Aug 22, 2023

zwangsheng Aug 23, 2023

waitinfuture left a comment

cfmcgrady commented Aug 23, 2023

zwangsheng commented Aug 23, 2023

[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method #1811

[CELEBORN-888][WORKER]Tweak the logic and add unit tests for the MemoryManager#currentServingState method #1811

Conversation

zwangsheng commented Aug 14, 2023 • edited Loading

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?

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

pan3793 commented Aug 14, 2023

zwangsheng commented Aug 14, 2023

zwangsheng commented Aug 14, 2023

cfmcgrady commented Aug 14, 2023

cfmcgrady commented Aug 16, 2023

zwangsheng commented Aug 16, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwangsheng Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waitinfuture left a comment

Choose a reason for hiding this comment

cfmcgrady commented Aug 23, 2023

zwangsheng commented Aug 23, 2023

zwangsheng commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

zwangsheng Aug 22, 2023 •

edited

Loading