From 6fa37013966eceb663108bcb72eaeab732a6171d Mon Sep 17 00:00:00 2001 From: samz406 Date: Sun, 12 Jul 2020 09:48:44 +0800 Subject: [PATCH] optimize SchedulerService.setScheduleState code (#3136) * Optimize SchedulerService.setScheduleState code * modify the test case to PowerMock * modify code smell * modify code smell Co-authored-by: dailidong --- .../api/service/SchedulerService.java | 23 +-- .../api/service/SchedulerServiceTest.java | 174 ++++++++++++++++-- 2 files changed, 166 insertions(+), 31 deletions(-) diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/SchedulerService.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/SchedulerService.java index f926fa9d4..ed856d509 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/SchedulerService.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/SchedulerService.java @@ -19,6 +19,7 @@ package org.apache.dolphinscheduler.api.service; import org.apache.dolphinscheduler.api.dto.ScheduleParam; import org.apache.dolphinscheduler.api.enums.Status; +import org.apache.dolphinscheduler.api.exceptions.ServiceException; import org.apache.dolphinscheduler.api.utils.PageInfo; import org.apache.dolphinscheduler.common.Constants; import org.apache.dolphinscheduler.common.enums.*; @@ -333,10 +334,9 @@ public class SchedulerService extends BaseService { if(scheduleStatus == ReleaseState.ONLINE){ // check process definition release state if(processDefinition.getReleaseState() != ReleaseState.ONLINE){ - ProcessDefinition definition = processDefinitionMapper.selectById(scheduleObj.getProcessDefinitionId()); logger.info("not release process definition id: {} , name : {}", processDefinition.getId(), processDefinition.getName()); - putMsg(result, Status.PROCESS_DEFINE_NOT_RELEASE, definition.getName()); + putMsg(result, Status.PROCESS_DEFINE_NOT_RELEASE, processDefinition.getName()); return result; } // check sub process definition release state @@ -380,7 +380,7 @@ public class SchedulerService extends BaseService { switch (scheduleStatus) { case ONLINE: { logger.info("Call master client set schedule online, project id: {}, flow id: {},host: {}", project.getId(), processDefinition.getId(), masterServers); - setSchedule(project.getId(), id); + setSchedule(project.getId(), scheduleObj); break; } case OFFLINE: { @@ -395,7 +395,7 @@ public class SchedulerService extends BaseService { } } catch (Exception e) { result.put(Constants.MSG, scheduleStatus == ReleaseState.ONLINE ? "set online failure" : "set offline failure"); - throw new RuntimeException(result.get(Constants.MSG).toString()); + throw new ServiceException(result.get(Constants.MSG).toString()); } putMsg(result, Status.SUCCESS); @@ -472,15 +472,10 @@ public class SchedulerService extends BaseService { return result; } - public void setSchedule(int projectId, int scheduleId) throws RuntimeException{ - logger.info("set schedule, project id: {}, scheduleId: {}", projectId, scheduleId); - + public void setSchedule(int projectId, Schedule schedule) { - Schedule schedule = processService.querySchedule(scheduleId); - if (schedule == null) { - logger.warn("process schedule info not exists"); - return; - } + int scheduleId = schedule.getId(); + logger.info("set schedule, project id: {}, scheduleId: {}", projectId, scheduleId); Date startDate = schedule.getStartTime(); Date endDate = schedule.getEndTime(); @@ -502,7 +497,7 @@ public class SchedulerService extends BaseService { * @param scheduleId schedule id * @throws RuntimeException runtime exception */ - public static void deleteSchedule(int projectId, int scheduleId) throws RuntimeException{ + public static void deleteSchedule(int projectId, int scheduleId) { logger.info("delete schedules of project id:{}, schedule id:{}", projectId, scheduleId); String jobName = QuartzExecutors.buildJobName(scheduleId); @@ -510,7 +505,7 @@ public class SchedulerService extends BaseService { if(!QuartzExecutors.getInstance().deleteJob(jobName, jobGroupName)){ logger.warn("set offline failure:projectId:{},scheduleId:{}",projectId,scheduleId); - throw new RuntimeException(String.format("set offline failure")); + throw new ServiceException("set offline failure"); } } diff --git a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SchedulerServiceTest.java b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SchedulerServiceTest.java index 9f6dca813..f75d808e5 100644 --- a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SchedulerServiceTest.java +++ b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/SchedulerServiceTest.java @@ -16,43 +16,183 @@ */ package org.apache.dolphinscheduler.api.service; -import org.apache.dolphinscheduler.api.ApiApplicationServer; import org.apache.dolphinscheduler.api.enums.Status; import org.apache.dolphinscheduler.common.Constants; import org.apache.dolphinscheduler.common.enums.ReleaseState; -import org.apache.dolphinscheduler.common.enums.UserType; +import org.apache.dolphinscheduler.common.model.Server; +import org.apache.dolphinscheduler.dao.entity.ProcessDefinition; import org.apache.dolphinscheduler.dao.entity.Project; +import org.apache.dolphinscheduler.dao.entity.Schedule; import org.apache.dolphinscheduler.dao.entity.User; +import org.apache.dolphinscheduler.dao.mapper.ProcessDefinitionMapper; +import org.apache.dolphinscheduler.dao.mapper.ProjectMapper; +import org.apache.dolphinscheduler.dao.mapper.ProjectUserMapper; +import org.apache.dolphinscheduler.dao.mapper.ScheduleMapper; +import org.apache.dolphinscheduler.service.process.ProcessService; +import org.apache.dolphinscheduler.service.quartz.QuartzExecutors; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; +import org.quartz.Scheduler; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.test.context.junit4.SpringRunner; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; import java.util.Map; -@RunWith(SpringRunner.class) -@SpringBootTest(classes = ApiApplicationServer.class) +@RunWith(PowerMockRunner.class) +@PrepareForTest(QuartzExecutors.class) + public class SchedulerServiceTest { - private static final Logger logger = LoggerFactory.getLogger(ExecutorServiceTest.class); - @Autowired + + @InjectMocks private SchedulerService schedulerService; + + @Autowired + private ExecutorService executorService; + + @Mock + private MonitorService monitorService; + + @Mock + private ProcessService processService; + + @Mock + private ScheduleMapper scheduleMapper; + + @Mock + private ProjectMapper projectMapper; + @Mock + private ProjectUserMapper projectUserMapper; + @Mock + private ProjectService projectService; + + @Mock + private ProcessDefinitionMapper processDefinitionMapper; + + @Mock + private QuartzExecutors quartzExecutors; + + @Mock + private Scheduler scheduler; + + + @Before + public void setUp() { + + quartzExecutors = PowerMockito.mock(QuartzExecutors.class); + PowerMockito.mockStatic(QuartzExecutors.class); + try { + PowerMockito.doReturn(quartzExecutors).when(QuartzExecutors.class, "getInstance"); + } catch (Exception e) { + e.printStackTrace(); + } + + } + @Test - public void testSetScheduleState(){ + public void testSetScheduleState() { + + + String projectName = "test"; User loginUser = new User(); - loginUser.setId(-1); - loginUser.setUserType(UserType.GENERAL_USER); + loginUser.setId(1); + Map result = new HashMap(); + Project project = getProject(projectName); + + + ProcessDefinition processDefinition = new ProcessDefinition(); + + Schedule schedule = new Schedule(); + schedule.setId(1); + schedule.setProcessDefinitionId(1); + schedule.setReleaseState(ReleaseState.OFFLINE); + + List masterServers = new ArrayList<>(); + masterServers.add(new Server()); + + Mockito.when(scheduleMapper.selectById(1)).thenReturn(schedule); + + Mockito.when(projectMapper.queryByName(projectName)).thenReturn(project); + + Mockito.when(processService.findProcessDefineById(1)).thenReturn(processDefinition); + + //hash no auth + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE); + + Mockito.when(projectService.hasProjectAndPerm(loginUser, project, result)).thenReturn(true); + //schedule not exists + result = schedulerService.setScheduleState(loginUser, projectName, 2, ReleaseState.ONLINE); + Assert.assertEquals(Status.SCHEDULE_CRON_NOT_EXISTS, result.get(Constants.STATUS)); + + //SCHEDULE_CRON_REALEASE_NEED_NOT_CHANGE + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.OFFLINE); + Assert.assertEquals(Status.SCHEDULE_CRON_REALEASE_NEED_NOT_CHANGE, result.get(Constants.STATUS)); + + //PROCESS_DEFINE_NOT_EXIST + schedule.setProcessDefinitionId(2); + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE); + Assert.assertEquals(Status.PROCESS_DEFINE_NOT_EXIST, result.get(Constants.STATUS)); + schedule.setProcessDefinitionId(1); + + // PROCESS_DEFINE_NOT_RELEASE + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE); + Assert.assertEquals(Status.PROCESS_DEFINE_NOT_RELEASE, result.get(Constants.STATUS)); + + processDefinition.setReleaseState(ReleaseState.ONLINE); + Mockito.when(processService.findProcessDefineById(1)).thenReturn(processDefinition); + + //MASTER_NOT_EXISTS + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE); + Assert.assertEquals(Status.MASTER_NOT_EXISTS, result.get(Constants.STATUS)); + + //set master + Mockito.when(monitorService.getServerListFromZK(true)).thenReturn(masterServers); + + //SUCCESS + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.ONLINE); + Assert.assertEquals(Status.SUCCESS, result.get(Constants.STATUS)); + + //OFFLINE + Mockito.when(quartzExecutors.deleteJob(null, null)).thenReturn(true); + result = schedulerService.setScheduleState(loginUser, projectName, 1, ReleaseState.OFFLINE); + Assert.assertEquals(Status.SUCCESS, result.get(Constants.STATUS)); + + } + + @Test + public void testDeleteSchedule() { + + Mockito.when(quartzExecutors.deleteJob("1", "1")).thenReturn(true); + Mockito.when(quartzExecutors.buildJobGroupName(1)).thenReturn("1"); + Mockito.when(quartzExecutors.buildJobName(1)).thenReturn("1"); + boolean flag = true; + try { + schedulerService.deleteSchedule(1, 1); + }catch (Exception e){ + flag = false; + } + Assert.assertTrue(flag); + + } + + private Project getProject(String name) { + Project project = new Project(); - project.setName("project_test1"); - project.setId(-1); + project.setName(name); + project.setUserId(1); - Map map = schedulerService.setScheduleState(loginUser, project.getName(), 44, ReleaseState.ONLINE); - Assert.assertEquals(Status.PROJECT_NOT_FOUNT, map.get(Constants.STATUS)); + return project; } } \ No newline at end of file -- GitLab