[CELEBORN-882][WORKER][METRICS] Add `Pause Push Data Time Count` Metrics & Dashboard Panel #1800

zwangsheng · 2023-08-08T10:07:36Z

What changes were proposed in this pull request?

Add PausePushDataTime Metrics

Why are the changes needed?

Count each celeborn worker pause time.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Cluster Test

zwangsheng · 2023-08-08T10:08:37Z

codecov · 2023-08-08T10:19:28Z

Codecov Report

Merging #1800 (7286c42) into main (d7e900f) will increase coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   46.49%   46.51%   +0.03%     
==========================================
  Files         164      164              
  Lines       10222    10228       +6     
  Branches      936      936              
==========================================
+ Hits         4752     4757       +5     
- Misses       5156     5157       +1     
  Partials      314      314

Files Changed	Coverage Δ
...cala/org/apache/celeborn/common/CelebornConf.scala	`87.49% <100.00%> (+0.03%)`	⬆️

... and 2 files with indirect coverage changes

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

pan3793 · 2023-08-09T00:23:15Z

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

@@ -65,7 +65,9 @@ public class MemoryManager {
  private final AtomicLong diskBufferCounter = new AtomicLong(0);
  private final LongAdder pausePushDataCounter = new LongAdder();
  private final LongAdder pausePushDataAndReplicateCounter = new LongAdder();
+  private final AtomicLong pausePushDataTime = new AtomicLong(0);


pan3793 · 2023-08-09T00:29:04Z

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

@@ -163,6 +166,7 @@ private MemoryManager(CelebornConf conf) {
                logger.info("Trigger action: RESUME PUSH and REPLICATE");
                memoryPressureListeners.forEach(
                    memoryPressureListener -> memoryPressureListener.onResume("all"));
+                pausePushDataTime.addAndGet(System.currentTimeMillis() - lastPauseTime);


the state machine does not look correct to me.

RESUM always be triggered after PAUSE_PUSH

what if the worker goes into PUSH_AND_REPLICATE_PAUSED?

zwangsheng · 2023-08-09T06:14:54Z

Marked as draft, should do some logic optimize before this.

FMX · 2023-08-10T03:47:45Z

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

@@ -140,6 +142,7 @@ private MemoryManager(CelebornConf conf) {
            if (lastState != servingState) {
              logger.info("Serving state transformed from {} to {}", lastState, servingState);
              if (servingState == ServingState.PUSH_PAUSED) {
+                lastPauseTime = System.currentTimeMillis();


PUSH_AND_REPLICATE_PAUSED will pause push data and Celeborn worker might changed to PUSH_AND_REPLICATE_PAUSED directly is memory pressure is high.

Got this. After the logic here clearly #1835 , will go on this pr.

zwangsheng · 2023-08-28T03:11:38Z

CI fail related to #1844, will rebase and test agine.

waitinfuture · 2023-08-28T06:52:01Z

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

+        long pauseSpendMills = System.currentTimeMillis() - pauseStartTime;
+        logger.info(
+            "Trigger action: RESUME PUSH and REPLICATE, pause push spent: " + pauseSpendMills);
+        pausePushDataTime.add(pauseSpendMills);


Consider the corner case that a worker enters the pause state but never goes out, then pausePushDataTime will never increase. Maybe we can increase pausePushDataTime every N successive pause states, as well as state changes from pause to non_pause.

Also, I think we should consider worker under high load only when the pause time exceeds some threshold, instead of whenever it enters pause state, as #1840 does, cc @pan3793

Consider the corner case that a worker enters the pause state but never goes out, then pausePushDataTime will never increase. Maybe we can increase pausePushDataTime every N successive pause states, as well as state changes from pause to non_pause.

This is a good idea.

waitinfuture · 2023-08-29T07:56:59Z

According to the snapshot, the metrics will monotonously increase. Is it better to define the metric as pause time in last xxx minutes?

zwangsheng · 2023-09-07T06:26:29Z

@waitinfuture
Sorry for late reply, current add logic to force append pause spent time after N TRIM action.

As your comment to show pause time in last xxx minutes, IMO, users may care about how long the cluster in back-pressure state over this time. pause time in last xxx minutes can tell us state in specifies the time frame, but not tell us macroscopically about the pause for the whole duration.

Current PR implementation can do both of the things mentioned above.

If you have any other ideas, please let me know.

waitinfuture · 2023-09-08T09:30:08Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

@@ -676,6 +676,8 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
  def metricsAppTopDiskUsageCount: Int = get(METRICS_APP_TOP_DISK_USAGE_COUNT)
  def metricsAppTopDiskUsageWindowSize: Int = get(METRICS_APP_TOP_DISK_USAGE_WINDOW_SIZE)
  def metricsAppTopDiskUsageInterval: Long = get(METRICS_APP_TOP_DISK_USAGE_INTERVAL)
+  def metricsWorkerForceAppendPauseSpentTimeThreshold: Int =
+    get(METRICS_WORKER_PAUSE_SPENT_TINE_FORCE_APPEND_THRESHOLD)


typo METRICS_WORKER_PAUSE_SPENT_TIME_FORCE_APPEND_THRESHOLD

waitinfuture · 2023-09-08T09:38:06Z

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

+        if (trimCounter.incrementAndGet() >= forceAppendPauseSpentTimeThreshold) {
+          logger.debug(
+              "Trigger action: TRIM for {} times, force to append pause spent time.",
+              trimCounter.incrementAndGet());


waitinfuture · 2023-09-08T09:49:43Z

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

@@ -65,8 +67,11 @@ public class MemoryManager {
  private final AtomicLong diskBufferCounter = new AtomicLong(0);
  private final LongAdder pausePushDataCounter = new LongAdder();
  private final LongAdder pausePushDataAndReplicateCounter = new LongAdder();
+  private final LongAdder pausePushDataTime = new LongAdder();


Since switchServingState will be called sequentially in a single thread, I think it's safe to use long/int for pausePushDataTime and trimCounter

zwangsheng · 2023-09-12T02:28:09Z

@waitinfuture Thanks for your review, made correction, PTAL

waitinfuture · 2023-09-12T06:32:02Z

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

  private volatile boolean isPaused = false;
+  private final AtomicInteger trimCounter = new AtomicInteger(0);


Does trimCounter need to be atomic?

waitinfuture

LGTM, thanks! Merging to main/0.3

…ics & Dashboard Panel ### What changes were proposed in this pull request? Add `PausePushDataTime ` Metrics ### Why are the changes needed? Count each celeborn worker pause time. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Cluster Test Closes #1800 from zwangsheng/CELEBORN-882. Lead-authored-by: zwangsheng <[email protected]> Co-authored-by: zwangsheng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]> (cherry picked from commit 03a3981) Signed-off-by: zky.zhoukeyong <[email protected]>

zwangsheng requested review from pan3793 and FMX August 8, 2023 10:07

zwangsheng self-assigned this Aug 8, 2023

pan3793 reviewed Aug 9, 2023

View reviewed changes

zwangsheng marked this pull request as draft August 9, 2023 06:14

FMX reviewed Aug 10, 2023

View reviewed changes

zwangsheng force-pushed the CELEBORN-882 branch from 5e75474 to 200fb00 Compare August 28, 2023 02:38

zwangsheng requested review from FMX and pan3793 August 28, 2023 02:39

zwangsheng marked this pull request as ready for review August 28, 2023 02:39

zwangsheng force-pushed the CELEBORN-882 branch from 200fb00 to 1c766b3 Compare August 28, 2023 05:33

waitinfuture reviewed Aug 28, 2023

View reviewed changes

zwangsheng force-pushed the CELEBORN-882 branch from 1c766b3 to f6cd42f Compare September 7, 2023 06:19

zwangsheng requested a review from waitinfuture September 7, 2023 09:13

waitinfuture reviewed Sep 8, 2023

View reviewed changes

pan3793 force-pushed the main branch 2 times, most recently from 58d0f48 to ada12a2 Compare September 8, 2023 11:49

zwangsheng and others added 4 commits September 11, 2023 20:55

rebase

aad66f1

Add config to force append pause spent time

0353d51

fix celeborn metrics configuration doc

d77af3e

fix comments

525bdd4

zwangsheng force-pushed the CELEBORN-882 branch from 9760414 to 525bdd4 Compare September 12, 2023 02:23

waitinfuture reviewed Sep 12, 2023

View reviewed changes

zwangsheng added 2 commits September 12, 2023 17:14

fix comments

c27f246

fix ci

7286c42

waitinfuture approved these changes Sep 12, 2023

View reviewed changes

waitinfuture closed this in 03a3981 Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-882][WORKER][METRICS] Add `Pause Push Data Time Count` Metrics & Dashboard Panel #1800

[CELEBORN-882][WORKER][METRICS] Add `Pause Push Data Time Count` Metrics & Dashboard Panel #1800

zwangsheng commented Aug 8, 2023 •

edited

Loading

zwangsheng commented Aug 8, 2023

codecov bot commented Aug 8, 2023 •

edited

Loading

pan3793 Aug 9, 2023

zwangsheng Aug 9, 2023

pan3793 Aug 9, 2023

zwangsheng Aug 9, 2023

pan3793 Aug 9, 2023

zwangsheng commented Aug 9, 2023

FMX Aug 10, 2023

zwangsheng Aug 24, 2023

zwangsheng commented Aug 28, 2023

waitinfuture Aug 28, 2023

zwangsheng Aug 28, 2023

waitinfuture commented Aug 29, 2023

zwangsheng commented Sep 7, 2023

waitinfuture Sep 8, 2023

waitinfuture Sep 8, 2023

waitinfuture Sep 8, 2023

zwangsheng commented Sep 12, 2023

waitinfuture Sep 12, 2023

zwangsheng Sep 12, 2023

waitinfuture left a comment

		private volatile boolean isPaused = false;
		private final AtomicInteger trimCounter = new AtomicInteger(0);

[CELEBORN-882][WORKER][METRICS] Add Pause Push Data Time Count Metrics & Dashboard Panel #1800

[CELEBORN-882][WORKER][METRICS] Add Pause Push Data Time Count Metrics & Dashboard Panel #1800

Conversation

zwangsheng commented Aug 8, 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?

zwangsheng commented Aug 8, 2023

codecov bot commented Aug 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwangsheng commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwangsheng commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waitinfuture commented Aug 29, 2023

zwangsheng commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zwangsheng commented Sep 12, 2023

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

[CELEBORN-882][WORKER][METRICS] Add `Pause Push Data Time Count` Metrics & Dashboard Panel #1800

[CELEBORN-882][WORKER][METRICS] Add `Pause Push Data Time Count` Metrics & Dashboard Panel #1800

zwangsheng commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading