From ccd3a553dab03475379b3640dd427866079cd8a4 Mon Sep 17 00:00:00 2001 From: Glenn Renfro Date: Mon, 14 Dec 2015 14:01:35 -0500 Subject: [PATCH] Code Review Changes --- .../repository/dao/JdbcTaskExecutionDao.java | 64 ++++--------------- .../repository/dao/MapTaskExecutionDao.java | 38 +++++------ .../support/JdbcTaskExplorerFactoryBean.java | 13 +++- .../JdbcTaskRepositoryFactoryBean.java | 13 +++- .../support/MapTaskExplorerFactoryBean.java | 13 +++- .../support/MapTaskRepositoryFactoryBean.java | 13 +++- .../support/SimpleTaskExplorerTests.java | 17 +++++ 7 files changed, 95 insertions(+), 76 deletions(-) diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/JdbcTaskExecutionDao.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/JdbcTaskExecutionDao.java index 157672301..4c494dae6 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/JdbcTaskExecutionDao.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/JdbcTaskExecutionDao.java @@ -20,7 +20,6 @@ import java.sql.SQLException; import java.sql.Types; import java.util.ArrayList; -import java.util.Collections; import java.util.Date; import java.util.HashSet; import java.util.List; @@ -29,11 +28,9 @@ import javax.sql.DataSource; import org.springframework.cloud.task.repository.TaskExecution; -import org.springframework.dao.DataAccessException; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.JdbcTemplate; -import org.springframework.jdbc.core.ResultSetExtractor; import org.springframework.jdbc.core.RowCallbackHandler; import org.springframework.jdbc.core.RowMapper; import org.springframework.util.Assert; @@ -83,7 +80,8 @@ public class JdbcTaskExecutionDao implements TaskExecutionDao { + "START_TIME, END_TIME, TASK_NAME, EXIT_CODE, " + "EXIT_MESSAGE, LAST_UPDATED, STATUS_CODE " + "from %PREFIX%EXECUTION where TASK_NAME = ? " - + "order by TASK_EXECUTION_ID"; + + "order by TASK_EXECUTION_ID " + + "LIMIT ? OFFSET ?"; final String FIND_TASK_NAMES = "SELECT distinct TASK_NAME from %PREFIX%EXECUTION order by TASK_NAME"; @@ -172,60 +170,22 @@ public long getTaskExecutionCount(String taskName) { @Override public Set findRunningTaskExecutions(String taskName) { final Set result = new HashSet(); - RowCallbackHandler handler = new RowCallbackHandler() { - @Override - public void processRow(ResultSet rs) throws SQLException { - TaskExecutionRowMapper mapper = new TaskExecutionRowMapper(); - result.add(mapper.mapRow(rs, 0)); - } - }; - jdbcTemplate.query(getQuery(FIND_RUNNING_TASK_EXECUTIONS), - new Object[] { taskName }, handler); - + List resultList = jdbcTemplate.query(getQuery(FIND_RUNNING_TASK_EXECUTIONS), + new Object[]{ taskName }, new TaskExecutionRowMapper()); + result.addAll(resultList); return result; } @Override public List getTaskExecutionsByName(String taskName, final int start, final int count) { - ResultSetExtractor> extractor = - new ResultSetExtractor>() { - - private List list = new ArrayList(); - - @Override - public List extractData(ResultSet rs) throws SQLException, - DataAccessException { - int rowNum = 0; - while (rowNum < start && rs.next()) { - rowNum++; - } - while (rowNum < start + count && rs.next()) { - RowMapper rowMapper = new TaskExecutionRowMapper(); - list.add(rowMapper.mapRow(rs, rowNum)); - rowNum++; - } - return list; - } - - }; - - List result = jdbcTemplate.query(getQuery(FIND_TASK_EXECUTIONS), - new Object[] { taskName }, extractor); - - return result; + return jdbcTemplate.query(getQuery(FIND_TASK_EXECUTIONS), + new Object[]{ taskName, count, start + 1 }, new TaskExecutionRowMapper()); } @Override public List getTaskNames() { - return jdbcTemplate.query(getQuery(FIND_TASK_NAMES), - new RowMapper() { - @Override - public String mapRow(ResultSet rs, int rowNum) - throws SQLException { - return rs.getString(1); - } - }); + return jdbcTemplate.queryForList(getQuery(FIND_TASK_NAMES), String.class); } private String getQuery(String base) { @@ -266,14 +226,12 @@ public void processRow(ResultSet rs) throws SQLException { jdbcTemplate.query(getQuery(FIND_PARAMS_FROM_ID), new Object[] { executionId }, handler); - return Collections.unmodifiableList(params); + return params; } /** * Re-usable mapper for {@link TaskExecution} instances. * - * @author Dave Syer - * */ private final class TaskExecutionRowMapper implements RowMapper { @@ -282,9 +240,9 @@ public TaskExecutionRowMapper() { @Override public TaskExecution mapRow(ResultSet rs, int rowNum) throws SQLException { - String id = rs.getString(1); + String id = rs.getString("TASK_EXECUTION_ID"); TaskExecution taskExecution=new TaskExecution(); - taskExecution.setExecutionId(rs.getString(1)); + taskExecution.setExecutionId(id); taskExecution.setStartTime(rs.getTimestamp("START_TIME")); taskExecution.setEndTime(rs.getTimestamp("END_TIME")); taskExecution.setExitCode(rs.getInt("EXIT_CODE")); diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/MapTaskExecutionDao.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/MapTaskExecutionDao.java index 24a656373..cacc171d0 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/MapTaskExecutionDao.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/dao/MapTaskExecutionDao.java @@ -18,11 +18,11 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; -import java.util.Iterator; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -69,49 +69,49 @@ public long getTaskExecutionCount(String taskName) { @Override public Set findRunningTaskExecutions(String taskName) { - Set result = new HashSet<>(); + Set result = getTaskExecutionTreeSet(); for (Map.Entry entry : taskExecutions.entrySet()) { if (entry.getValue().getTaskName().equals(taskName) && entry.getValue().getEndTime() == null) { result.add(entry.getValue()); } } - return Collections.unmodifiableSet(result); + return result; } @Override public List getTaskExecutionsByName(String taskName, int start, int count) { List result = new ArrayList<>(); - Set filteredSet = new HashSet<>(); + Set filteredSet = getTaskExecutionTreeSet(); for (Map.Entry entry : taskExecutions.entrySet()) { if (entry.getValue().getTaskName().equals(taskName)) { filteredSet.add(entry.getValue()); } } - int rowNum = 0; - Iterator rs = filteredSet.iterator(); - while (rowNum < start && rs.hasNext()) { - rs.next(); - rowNum++; - } - while (rowNum < start + count && rs.hasNext()) { - result.add(rs.next()); - rowNum++; - } - - return Collections.unmodifiableList(result); + result.addAll(filteredSet); + return result.subList(start, start + count); } @Override public List getTaskNames() { - Set result = new HashSet<>(); + Set result = new TreeSet<>(); for (Map.Entry entry : taskExecutions.entrySet()) { result.add(entry.getValue().getTaskName()); } - return Collections.unmodifiableList(new ArrayList(result)); + return new ArrayList(result); } public Map getTaskExecutions() { return Collections.unmodifiableMap(taskExecutions); } + + private TreeSet getTaskExecutionTreeSet() { + return new TreeSet(new Comparator() { + @Override + public int compare(TaskExecution e1, TaskExecution e2) { + return e1.getExecutionId().compareTo(e2.getExecutionId()); + } + }); + } + } diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskExplorerFactoryBean.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskExplorerFactoryBean.java index 5f25eac1b..81e4a33a7 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskExplorerFactoryBean.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskExplorerFactoryBean.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.FactoryBean; import org.springframework.cloud.task.repository.TaskExplorer; import org.springframework.cloud.task.repository.dao.JdbcTaskExecutionDao; import org.springframework.cloud.task.repository.dao.TaskExecutionDao; @@ -30,7 +31,7 @@ * * @author Glenn Renfro */ -public class JdbcTaskExplorerFactoryBean { +public class JdbcTaskExplorerFactoryBean implements FactoryBean{ public static final String DEFAULT_TABLE_PREFIX = "TASK_"; @@ -70,6 +71,16 @@ public TaskExplorer getObject(){ return taskExplorer; } + @Override + public Class getObjectType() { + return TaskExplorer.class; + } + + @Override + public boolean isSingleton() { + return true; + } + private TaskExecutionDao createJdbcTaskExecutionDao() { JdbcTaskExecutionDao dao = new JdbcTaskExecutionDao(dataSource); dao.setTablePrefix(tablePrefix); diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskRepositoryFactoryBean.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskRepositoryFactoryBean.java index 1ccca7acb..2a78f48c2 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskRepositoryFactoryBean.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/JdbcTaskRepositoryFactoryBean.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.FactoryBean; import org.springframework.cloud.task.repository.TaskRepository; import org.springframework.cloud.task.repository.dao.JdbcTaskExecutionDao; import org.springframework.cloud.task.repository.dao.TaskExecutionDao; @@ -31,7 +32,7 @@ * * @author Glenn Renfro */ -public class JdbcTaskRepositoryFactoryBean { +public class JdbcTaskRepositoryFactoryBean implements FactoryBean{ public static final String DEFAULT_TABLE_PREFIX = "TASK_"; @@ -71,6 +72,16 @@ public TaskRepository getObject(){ return taskRepository; } + @Override + public Class getObjectType() { + return TaskRepository.class; + } + + @Override + public boolean isSingleton() { + return true; + } + private TaskExecutionDao createJdbcTaskExecutionDao() { JdbcTaskExecutionDao dao = new JdbcTaskExecutionDao(dataSource); dao.setTablePrefix(tablePrefix); diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskExplorerFactoryBean.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskExplorerFactoryBean.java index fb749ad47..83ddd1c71 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskExplorerFactoryBean.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskExplorerFactoryBean.java @@ -18,6 +18,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.FactoryBean; import org.springframework.cloud.task.repository.TaskExplorer; import org.springframework.cloud.task.repository.dao.MapTaskExecutionDao; @@ -27,7 +28,7 @@ * * @author Glenn Renfro */ -public class MapTaskExplorerFactoryBean { +public class MapTaskExplorerFactoryBean implements FactoryBean{ private static final Log logger = LogFactory.getLog(MapTaskExplorerFactoryBean.class); @@ -47,4 +48,14 @@ public TaskExplorer getObject(){ return taskExplorer; } + @Override + public Class getObjectType() { + return TaskExplorer.class; + } + + @Override + public boolean isSingleton() { + return true; + } + } diff --git a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskRepositoryFactoryBean.java b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskRepositoryFactoryBean.java index e3698a8d5..706e03c25 100644 --- a/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskRepositoryFactoryBean.java +++ b/spring-cloud-task-core/src/main/java/org/springframework/cloud/task/repository/support/MapTaskRepositoryFactoryBean.java @@ -18,6 +18,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.FactoryBean; import org.springframework.cloud.task.repository.TaskRepository; import org.springframework.cloud.task.repository.dao.MapTaskExecutionDao; @@ -28,7 +29,7 @@ * * @author Glenn Renfro */ -public class MapTaskRepositoryFactoryBean { +public class MapTaskRepositoryFactoryBean implements FactoryBean{ private static final Log logger = LogFactory.getLog(MapTaskRepositoryFactoryBean.class); @@ -47,4 +48,14 @@ public MapTaskRepositoryFactoryBean(){ public TaskRepository getObject(){ return taskRepository; } + + @Override + public Class getObjectType() { + return TaskRepository.class; + } + + @Override + public boolean isSingleton() { + return true; + } } diff --git a/spring-cloud-task-core/src/test/java/org/springframework/cloud/task/repository/support/SimpleTaskExplorerTests.java b/spring-cloud-task-core/src/test/java/org/springframework/cloud/task/repository/support/SimpleTaskExplorerTests.java index 0c47eb417..b98beb04c 100644 --- a/spring-cloud-task-core/src/test/java/org/springframework/cloud/task/repository/support/SimpleTaskExplorerTests.java +++ b/spring-cloud-task-core/src/test/java/org/springframework/cloud/task/repository/support/SimpleTaskExplorerTests.java @@ -117,6 +117,23 @@ public void getTaskExecution() { } } + @Test + public void taskExecutionNotFound() { + final int TEST_COUNT = 5; + Map expectedResults = new HashMap<>(); + for (int i = 0; i < TEST_COUNT; i++) { + TaskExecution expectedTaskExecution = createAndSaveTaskExecution(); + expectedResults.put(expectedTaskExecution.getExecutionId(), + expectedTaskExecution); + } + + TaskExecution actualTaskExecution = + taskExplorer.getTaskExecution("NO_EXECUTION_PRESENT"); + assertNull(String.format( + "expected null for actualTaskExecution %s", testType), + actualTaskExecution); + } + @Test public void getTaskCountByTaskName() { final int TEST_COUNT = 5;