Skip to content

Commit 78dd6ad

Browse files
sanjana2505006sanjana2505006
committed
Fix maintainer feedback: revert unnecessary change and improve enum usage
- Add back empty line in AbstractTaskStateAction.java line 199 as requested - Add incTaskInstanceByLifecycleEvent method to TaskMetrics to use TaskLifecycleEventType directly - Update WorkflowEventBusFireWorker to use new method instead of string comparisons - Add comprehensive tests for new lifecycle event method Addresses feedback from SbloodyS and ruanwenjun on PR #18038
1 parent e30a3d3 commit 78dd6ad

4 files changed

Lines changed: 138 additions & 24 deletions

File tree

‎dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/WorkflowEventBusFireWorker.java‎

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -166,30 +166,12 @@ private void recordTaskInstanceMetrics(AbstractLifecycleEvent event) {
166166
return;
167167
}
168168

169-
switch ((org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType) event
170-
.getEventType()) {
171-
case DISPATCHED:
172-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("dispatch");
173-
break;
174-
case SUCCEEDED:
175-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("success");
176-
break;
177-
case FAILED:
178-
case FATAL:
179-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("fail");
180-
break;
181-
case KILLED:
182-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("kill");
183-
break;
184-
case RETRY:
185-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("retry");
186-
break;
187-
case TIMEOUT:
188-
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics.incTaskInstanceByState("timeout");
189-
break;
190-
default:
191-
break;
192-
}
169+
final org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType taskLifecycleEventType =
170+
(org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType) event
171+
.getEventType();
172+
173+
org.apache.dolphinscheduler.server.master.metrics.TaskMetrics
174+
.incTaskInstanceByLifecycleEvent(taskLifecycleEventType);
193175
}
194176

195177
}

‎dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/engine/task/statemachine/AbstractTaskStateAction.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ private void persistentTaskInstanceKilledEventToDB(final ITaskExecutionRunnable
196196
taskInstance.setState(TaskExecutionStatus.KILL);
197197
taskInstance.setEndTime(taskKilledEvent.getEndTime());
198198
taskInstanceDao.updateById(taskInstance);
199+
199200
}
200201

201202
@Override

‎dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717

1818
package org.apache.dolphinscheduler.server.master.metrics;
1919

20+
import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType;
21+
2022
import java.util.HashMap;
2123
import java.util.Map;
2224
import java.util.Set;
@@ -50,6 +52,21 @@ public class TaskMetrics {
5052

5153
}
5254

55+
private final Counter taskDispatchCounter =
56+
Counter.builder("ds.task.dispatch.count")
57+
.description("Task dispatch count")
58+
.register(Metrics.globalRegistry);
59+
60+
private final Counter taskDispatchFailCounter =
61+
Counter.builder("ds.task.dispatch.failure.count")
62+
.description("Task dispatch failures count, retried ones included")
63+
.register(Metrics.globalRegistry);
64+
65+
private final Counter taskDispatchErrorCounter =
66+
Counter.builder("ds.task.dispatch.error.count")
67+
.description("Number of errors during task dispatch")
68+
.register(Metrics.globalRegistry);
69+
5370
public synchronized void registerTaskPrepared(Supplier<Number> consumer) {
5471
Gauge.builder("ds.task.prepared", consumer)
5572
.description("Task prepared count")
@@ -63,4 +80,45 @@ public void incTaskInstanceByState(final String state) {
6380
taskInstanceCounters.get(state).increment();
6481
}
6582

83+
public void incTaskDispatchFailed(int failedCount) {
84+
taskDispatchFailCounter.increment(failedCount);
85+
}
86+
87+
public void incTaskDispatchError() {
88+
taskDispatchErrorCounter.increment();
89+
}
90+
91+
public void incTaskDispatch() {
92+
taskDispatchCounter.increment();
93+
}
94+
95+
public void incTaskInstanceByLifecycleEvent(final TaskLifecycleEventType eventType) {
96+
if (eventType == null) {
97+
return;
98+
}
99+
switch (eventType) {
100+
case DISPATCHED:
101+
incTaskInstanceByState("dispatch");
102+
break;
103+
case SUCCEEDED:
104+
incTaskInstanceByState("success");
105+
break;
106+
case FAILED:
107+
case FATAL:
108+
incTaskInstanceByState("fail");
109+
break;
110+
case KILLED:
111+
incTaskInstanceByState("kill");
112+
break;
113+
case RETRY:
114+
incTaskInstanceByState("retry");
115+
break;
116+
case TIMEOUT:
117+
incTaskInstanceByState("timeout");
118+
break;
119+
default:
120+
break;
121+
}
122+
}
123+
66124
}

‎dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetricsTest.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import static org.junit.jupiter.api.Assertions.assertNotNull;
2222
import static org.junit.jupiter.api.Assertions.assertTrue;
2323

24+
import org.apache.dolphinscheduler.server.master.engine.task.lifecycle.TaskLifecycleEventType;
25+
2426
import java.util.Arrays;
2527
import java.util.List;
2628

@@ -81,4 +83,75 @@ void testRegisterTaskPrepared() {
8183
"Task prepared gauge should be registered");
8284
}
8385

86+
@Test
87+
void testIncTaskInstanceByLifecycleEvent_validEvents() {
88+
// Test each lifecycle event that maps to a metric
89+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.DISPATCHED);
90+
Counter dispatchCounter = Metrics.globalRegistry.find("ds.task.instance.count")
91+
.tag("state", "dispatch")
92+
.counter();
93+
assertNotNull(dispatchCounter);
94+
assertEquals(1, dispatchCounter.count(), 0.001, "Dispatch counter should be incremented");
95+
96+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.SUCCEEDED);
97+
Counter successCounter = Metrics.globalRegistry.find("ds.task.instance.count")
98+
.tag("state", "success")
99+
.counter();
100+
assertNotNull(successCounter);
101+
assertEquals(1, successCounter.count(), 0.001, "Success counter should be incremented");
102+
103+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.FAILED);
104+
Counter failCounter = Metrics.globalRegistry.find("ds.task.instance.count")
105+
.tag("state", "fail")
106+
.counter();
107+
assertNotNull(failCounter);
108+
assertEquals(1, failCounter.count(), 0.001, "Fail counter should be incremented");
109+
110+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.FATAL);
111+
assertEquals(2, failCounter.count(), 0.001, "Fail counter should be incremented for FATAL event");
112+
113+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.KILLED);
114+
Counter killCounter = Metrics.globalRegistry.find("ds.task.instance.count")
115+
.tag("state", "kill")
116+
.counter();
117+
assertNotNull(killCounter);
118+
assertEquals(1, killCounter.count(), 0.001, "Kill counter should be incremented");
119+
120+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.RETRY);
121+
Counter retryCounter = Metrics.globalRegistry.find("ds.task.instance.count")
122+
.tag("state", "retry")
123+
.counter();
124+
assertNotNull(retryCounter);
125+
assertEquals(1, retryCounter.count(), 0.001, "Retry counter should be incremented");
126+
127+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.TIMEOUT);
128+
Counter timeoutCounter = Metrics.globalRegistry.find("ds.task.instance.count")
129+
.tag("state", "timeout")
130+
.counter();
131+
assertNotNull(timeoutCounter);
132+
assertEquals(1, timeoutCounter.count(), 0.001, "Timeout counter should be incremented");
133+
}
134+
135+
@Test
136+
void testIncTaskInstanceByLifecycleEvent_unmappedEvents() {
137+
// Test events that don't map to metrics (should not increment any counter)
138+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.START);
139+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.RUNNING);
140+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.RUNTIME_CONTEXT_CHANGED);
141+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.PAUSE);
142+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.PAUSED);
143+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.FAILOVER);
144+
TaskMetrics.incTaskInstanceByLifecycleEvent(TaskLifecycleEventType.KILL);
145+
146+
// Verify no counters were incremented for unmapped events
147+
// We can't easily verify this without checking all counters, but the test should pass without exceptions
148+
}
149+
150+
@Test
151+
void testIncTaskInstanceByLifecycleEvent_nullEvent() {
152+
// Test that null event doesn't cause issues
153+
TaskMetrics.incTaskInstanceByLifecycleEvent(null);
154+
// Should not throw any exception
155+
}
156+
84157
}

0 commit comments

Comments
 (0)