From 672012d67cd383af801e7a516da300a8de0a0493 Mon Sep 17 00:00:00 2001 From: Faaya Fulas Date: Wed, 15 Jul 2020 17:57:53 -0400 Subject: [PATCH 1/4] First pass --- .../SingularityRequestHistoryQuery.java | 99 +++++++++++++++++++ .../singularity/client/SingularityClient.java | 39 ++++++-- .../data/history/AbstractHistoryJDBI.java | 69 +++++++++++++ .../singularity/data/history/HistoryJDBI.java | 2 + .../data/history/HistoryManager.java | 2 + .../data/history/JDBIHistoryManager.java | 8 +- .../data/history/MySQLHistoryJDBI.java | 23 +++-- .../data/history/NoopHistoryManager.java | 2 + .../data/history/PostgresHistoryJDBI.java | 23 +++-- .../data/history/RequestHistoryHelper.java | 75 +++++++++++--- .../resources/HistoryResource.java | 28 +++++- .../singularity/SingularityHistoryTest.java | 29 +++++- .../singularity/data/BlendedHistoryTest.java | 27 +++-- 13 files changed, 371 insertions(+), 55 deletions(-) create mode 100644 SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java diff --git a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java new file mode 100644 index 0000000000..ab1811f6a3 --- /dev/null +++ b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java @@ -0,0 +1,99 @@ +package com.hubspot.singularity; + +import com.google.common.base.Predicate; +import com.google.common.collect.ComparisonChain; +import java.util.Comparator; +import java.util.Optional; + +public class SingularityRequestHistoryQuery { + private final String requestId; + private final Optional createdBefore; + private final Optional createdAfter; + private final Optional orderDirection; + + public SingularityRequestHistoryQuery( + String requestId, + Optional createdBefore, + Optional createdAfter, + Optional orderDirection + ) { + this.requestId = requestId; + this.createdBefore = createdBefore; + this.createdAfter = createdAfter; + this.orderDirection = orderDirection; + } + + public String getRequestId() { + return requestId; + } + + public Optional getCreatedBefore() { + return createdBefore; + } + + public Optional getCreatedAfter() { + return createdAfter; + } + + public Optional getOrderDirection() { + return orderDirection; + } + + public Predicate getHistoryFilter() { + return new Predicate() { + + @Override + public boolean apply(SingularityRequestHistory input) { + if (!requestId.equals(input.getRequest().getId())) { + return false; + } + + if (createdAfter.isPresent() && createdAfter.get() >= input.getCreatedAt()) { + return false; + } + + if (createdBefore.isPresent() && createdBefore.get() <= input.getCreatedAt()) { + return false; + } + + return true; + } + }; + } + + public Comparator getComparator() { + final OrderDirection localOrderDirection = orderDirection.orElse(OrderDirection.DESC); + + return new Comparator() { + + @Override + public int compare(SingularityRequestHistory o1, SingularityRequestHistory o2) { + ComparisonChain chain = ComparisonChain.start(); + + if (localOrderDirection == OrderDirection.ASC) { + chain = chain.compare(o1.getCreatedAt(), o2.getCreatedAt()); + } else { + chain = chain.compare(o2.getCreatedAt(), o1.getCreatedAt()); + } + + return chain.compare(o1.getRequest().getId(), o2.getRequest().getId()).result(); + } + }; + } + + @Override + public String toString() { + return ( + "SingularityTaskHistoryQuery{" + + "requestId=" + + requestId + + ", createdBefore=" + + createdBefore + + ", createdAfter=" + + createdAfter + + ", orderDirection=" + + orderDirection + + '}' + ); + } +} diff --git a/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java b/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java index 86118c1aa0..195ca2a42a 100644 --- a/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java +++ b/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java @@ -47,6 +47,7 @@ import com.hubspot.singularity.SingularityRequestCleanup; import com.hubspot.singularity.SingularityRequestGroup; import com.hubspot.singularity.SingularityRequestHistory; +import com.hubspot.singularity.SingularityRequestHistoryQuery; import com.hubspot.singularity.SingularityRequestParent; import com.hubspot.singularity.SingularityRequestWithState; import com.hubspot.singularity.SingularityS3Log; @@ -1669,6 +1670,10 @@ public void deleteSlave(String slaveId) { * * @param requestId * Request ID to look up + * @param createdBefore + * Long millis to filter request histories created before + * @param createdAfter + * Long millis to filter request histories created after * @param count * Number of items to return per page * @param page @@ -1677,17 +1682,27 @@ public void deleteSlave(String slaveId) { * A list of {@link SingularityRequestHistory} */ public Collection getHistoryForRequest( - String requestId, - Optional count, - Optional page + String requestId, + Optional createdBefore, + Optional createdAfter, + Optional count, + Optional page ) { final Function requestUri = host -> - String.format(REQUEST_HISTORY_FORMAT, getApiBase(host), requestId); + String.format(REQUEST_HISTORY_FORMAT, getApiBase(host), requestId); Optional> maybeQueryParams = Optional.empty(); ImmutableMap.Builder queryParamsBuilder = ImmutableMap.builder(); + if (createdBefore.isPresent()) { + queryParamsBuilder.put("createdBefore", createdBefore.get()); + } + + if (createdAfter.isPresent()) { + queryParamsBuilder.put("createdAfter", createdAfter.get()); + } + if (count.isPresent()) { queryParamsBuilder.put("count", count.get()); } @@ -1702,13 +1717,21 @@ public Collection getHistoryForRequest( } return getCollectionWithParams( - requestUri, - "request history", - maybeQueryParams, - REQUEST_HISTORY_COLLECTION + requestUri, + "request history", + maybeQueryParams, + REQUEST_HISTORY_COLLECTION ); } + public Collection getHistoryForRequest( + String requestId, + Optional count, + Optional page + ) { + return getHistoryForRequest(requestId, Optional.empty(), Optional.empty(), count, page); + } + // // Inactive/Bad Slaves // diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java index 9f0115e24e..f06dd7b2e8 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java @@ -2,7 +2,9 @@ import com.hubspot.singularity.ExtendedTaskState; import com.hubspot.singularity.OrderDirection; +import com.hubspot.singularity.SingularityRequestHistory; import com.hubspot.singularity.SingularityTaskIdHistory; +import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -28,6 +30,32 @@ default void addWhereOrAnd(StringBuilder sqlBuilder, boolean shouldUseWhere) { } } + String getRequestHistoryBaseQuery(); + + default void applyRequestHistoryBaseQuery( + StringBuilder sqlBuilder, + Map binds, + String requestId, + Optional createdBefore, + Optional createdAfter + ) { + addWhereOrAnd(sqlBuilder, binds.isEmpty()); + sqlBuilder.append("requestId = :requestId"); + binds.put("requestId", requestId); + + if (createdBefore.isPresent()) { + addWhereOrAnd(sqlBuilder, binds.isEmpty()); + sqlBuilder.append("createdAt < :createdBefore"); + binds.put("createdBefore", new Date(createdBefore.get())); + } + + if (createdAfter.isPresent()) { + addWhereOrAnd(sqlBuilder, binds.isEmpty()); + sqlBuilder.append("createdAt > :createdAfter"); + binds.put("createdAfter", new Date(createdAfter.get())); + } + } + default void applyTaskIdHistoryBaseQuery( StringBuilder sqlBuilder, Map binds, @@ -156,6 +184,47 @@ default List getTaskIdHistory( return query.mapTo(SingularityTaskIdHistory.class).list(); } + default List getRequestHistory( + String requestId, + Optional createdBefore, + Optional createdAfter, + String orderDirection, + Integer limitStart, + Integer limitCount + ) { + final Map binds = new HashMap<>(); + final StringBuilder sqlBuilder = new StringBuilder(getRequestHistoryBaseQuery()); + + applyRequestHistoryBaseQuery( + sqlBuilder, + binds, + requestId, + createdBefore, + createdAfter + ); + + sqlBuilder.append(" ORDER BY createdAt "); + sqlBuilder.append(orderDirection); + if (limitCount != null) { + sqlBuilder.append(" LIMIT :limitCount"); + binds.put("limitCount", limitCount); + } + + if (limitStart != null) { + sqlBuilder.append(" OFFSET :limitStart "); + binds.put("limitStart", limitStart); + } + + final String sql = sqlBuilder.toString(); + + LOG.trace("Generated sql for request history search: {}, binds: {}", sql, binds); + + Query query = getHandle().createQuery(sql); + binds.forEach(query::bind); + + return query.mapTo(SingularityRequestHistory.class).list(); + } + default int getTaskIdHistoryCount( Optional requestId, Optional deployId, diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryJDBI.java index cd6349e8d2..d29acd2b48 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryJDBI.java @@ -62,6 +62,8 @@ List getDeployHistoryForRequest( List getRequestHistory( String requestId, + Optional createdBefore, + Optional createdAfter, String orderDirection, Integer limitStart, Integer limitCount diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryManager.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryManager.java index b35669f5a8..9359d75ce0 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryManager.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/HistoryManager.java @@ -61,6 +61,8 @@ int getTaskIdHistoryCount( List getRequestHistory( String requestId, + Optional createdBefore, + Optional createdAfter, Optional orderDirection, Integer limitStart, Integer limitCount diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/JDBIHistoryManager.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/JDBIHistoryManager.java index 0ccdbe7b15..8693d728f3 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/JDBIHistoryManager.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/JDBIHistoryManager.java @@ -271,20 +271,26 @@ private String getOrderDirection(Optional orderDirection) { @Override public List getRequestHistory( String requestId, + Optional createdBefore, + Optional createdAfter, Optional orderDirection, Integer limitStart, Integer limitCount ) { List singularityRequestHistoryList = history.getRequestHistory( requestId, + createdBefore, + createdAfter, getOrderDirection(orderDirection), limitStart, limitCount ); if (LOG.isTraceEnabled()) { LOG.trace( - "getRequestHistory requestId {}, orderDirection {}, limitStart {} , limitCount {}, requestHistory{}", + "getRequestHistory requestId {}, createdBefore {}, createdAfter {}, orderDirection {}, limitStart {} , limitCount {}, requestHistory{}", requestId, + createdBefore, + createdAfter, orderDirection, limitStart, limitCount, diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java index 527d000b7b..aab8cec3da 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java @@ -93,15 +93,15 @@ List getDeployHistoryForRequest( @SqlQuery("SELECT COUNT(*) FROM deployHistory WHERE requestId = :requestId") int getDeployHistoryForRequestCount(@Bind("requestId") String requestId); - @SqlQuery( - "SELECT json, request, createdAt, requestState, user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt LIMIT :limitStart, :limitCount" - ) - List getRequestHistory( - @Bind("requestId") String requestId, - @Define("orderDirection") String orderDirection, - @Bind("limitStart") Integer limitStart, - @Bind("limitCount") Integer limitCount - ); + // @SqlQuery( + // "SELECT json, request, createdAt, requestState, user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt LIMIT :limitStart, :limitCount" + // ) + // List getRequestHistory( + // @Bind("requestId") String requestId, + // @Define("orderDirection") String orderDirection, + // @Bind("limitStart") Integer limitStart, + // @Bind("limitCount") Integer limitCount + // ); @SqlQuery("SELECT COUNT(*) FROM requestHistory WHERE requestId = :requestId") int getRequestHistoryCount(@Bind("requestId") String requestId); @@ -253,5 +253,10 @@ void setDeployJson( @Bind("json") @Json SingularityDeployHistory deployHistory ); + @Override + default String getRequestHistoryBaseQuery() { + return "SELECT json, request, createdAt, requestState, user, message FROM requestHistory"; + } + default void close() {} } diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/NoopHistoryManager.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/NoopHistoryManager.java index 00aa5b69a1..c474f40ced 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/NoopHistoryManager.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/NoopHistoryManager.java @@ -105,6 +105,8 @@ public Optional getTaskHistoryByRunId( @Override public List getRequestHistory( String requestId, + Optional createdBefore, + Optional createdAfter, Optional orderDirection, Integer limitStart, Integer limitCount diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java index 36281c7fa6..ddc8c4c5d5 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java @@ -93,15 +93,15 @@ List getDeployHistoryForRequest( @SqlQuery("SELECT COUNT(*) FROM deployHistory WHERE requestId = :requestId") int getDeployHistoryForRequestCount(@Bind("requestId") String requestId); - @SqlQuery( - "SELECT json, request, createdAt, requestState, f_user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt OFFSET :limitStart LIMIT :limitCount" - ) - List getRequestHistory( - @Bind("requestId") String requestId, - @Define("orderDirection") String orderDirection, - @Bind("limitStart") Integer limitStart, - @Bind("limitCount") Integer limitCount - ); + // @SqlQuery( + // "SELECT json, request, createdAt, requestState, f_user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt OFFSET :limitStart LIMIT :limitCount" + // ) + // List getRequestHistory( + // @Bind("requestId") String requestId, + // @Define("orderDirection") String orderDirection, + // @Bind("limitStart") Integer limitStart, + // @Bind("limitCount") Integer limitCount + // ); @SqlQuery("SELECT COUNT(*) FROM requestHistory WHERE requestId = :requestId") int getRequestHistoryCount(@Bind("requestId") String requestId); @@ -253,5 +253,10 @@ void setDeployJson( @Bind("json") @Json SingularityDeployHistory deployHistory ); + @Override + default String getRequestHistoryBaseQuery() { + return "SELECT json, request, createdAt, requestState, f_user, message FROM requestHistory"; + } + default void close() {} } diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/RequestHistoryHelper.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/RequestHistoryHelper.java index 736922981d..d40f490bbd 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/RequestHistoryHelper.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/RequestHistoryHelper.java @@ -1,10 +1,15 @@ package com.hubspot.singularity.data.history; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.inject.Inject; import com.google.inject.Singleton; import com.hubspot.mesos.JavaUtils; import com.hubspot.singularity.OrderDirection; import com.hubspot.singularity.SingularityRequestHistory; +import com.hubspot.singularity.SingularityRequestHistoryQuery; +import com.hubspot.singularity.SingularityTaskHistoryQuery; +import com.hubspot.singularity.SingularityTaskIdHistory; import com.hubspot.singularity.config.SingularityConfiguration; import com.hubspot.singularity.data.RequestManager; import java.util.Collections; @@ -13,7 +18,7 @@ @Singleton public class RequestHistoryHelper - extends BlendedHistoryHelper { + extends BlendedHistoryHelper { private final RequestManager requestManager; private final HistoryManager historyManager; @@ -29,25 +34,32 @@ public RequestHistoryHelper( } @Override - protected List getFromZk(String requestId) { + protected List getFromZk( + SingularityRequestHistoryQuery query + ) { List requestHistory = requestManager.getRequestHistory( - requestId + query.getRequestId() + ); + final List filteredHistory = Lists.newArrayList( + Iterables.filter(requestHistory, query.getHistoryFilter()) ); - Collections.sort(requestHistory); + Collections.sort(filteredHistory, query.getComparator()); - return requestHistory; + return filteredHistory; } @Override protected List getFromHistory( - String requestId, + SingularityRequestHistoryQuery query, int historyStart, int numFromHistory ) { return historyManager.getRequestHistory( - requestId, - Optional.of(OrderDirection.DESC), + query.getRequestId(), + query.getCreatedBefore(), + query.getCreatedAfter(), + query.getOrderDirection(), historyStart, numFromHistory ); @@ -55,19 +67,42 @@ protected List getFromHistory( public Optional getFirstHistory(String requestId) { Optional firstHistory = JavaUtils.getFirst( - historyManager.getRequestHistory(requestId, Optional.of(OrderDirection.ASC), 0, 1) + historyManager.getRequestHistory( + requestId, + Optional.empty(), + Optional.empty(), + Optional.of(OrderDirection.ASC), + 0, + 1 + ) ); if (firstHistory.isPresent()) { return firstHistory; } - return JavaUtils.getLast(getFromZk(requestId)); + return JavaUtils.getLast( + getFromZk( + new SingularityRequestHistoryQuery( + requestId, + Optional.empty(), + Optional.empty(), + Optional.empty() + ) + ) + ); } public Optional getLastHistory(String requestId) { Optional lastHistory = JavaUtils.getFirst( - getFromZk(requestId) + getFromZk( + new SingularityRequestHistoryQuery( + requestId, + Optional.empty(), + Optional.empty(), + Optional.empty() + ) + ) ); if (lastHistory.isPresent()) { @@ -75,19 +110,29 @@ public Optional getLastHistory(String requestId) { } return JavaUtils.getFirst( - historyManager.getRequestHistory(requestId, Optional.of(OrderDirection.DESC), 0, 1) + historyManager.getRequestHistory( + requestId, + Optional.empty(), + Optional.empty(), + Optional.of(OrderDirection.DESC), + 0, + 1 + ) ); } @Override - protected Optional getTotalCount(String requestId, boolean canSkipZk) { + protected Optional getTotalCount( + SingularityRequestHistoryQuery query, + boolean canSkipZk + ) { int numFromZk; if (sqlEnabled && canSkipZk) { numFromZk = 0; } else { - numFromZk = requestManager.getRequestHistory(requestId).size(); + numFromZk = requestManager.getRequestHistory(query.getRequestId()).size(); } - int numFromHistory = historyManager.getRequestHistoryCount(requestId); + int numFromHistory = historyManager.getRequestHistoryCount(query.getRequestId()); return Optional.of(numFromZk + numFromHistory); } diff --git a/SingularityService/src/main/java/com/hubspot/singularity/resources/HistoryResource.java b/SingularityService/src/main/java/com/hubspot/singularity/resources/HistoryResource.java index d0ee8c9691..95d60c69ec 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/resources/HistoryResource.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/resources/HistoryResource.java @@ -12,6 +12,7 @@ import com.hubspot.singularity.SingularityDeployKey; import com.hubspot.singularity.SingularityPaginatedResponse; import com.hubspot.singularity.SingularityRequestHistory; +import com.hubspot.singularity.SingularityRequestHistoryQuery; import com.hubspot.singularity.SingularityTask; import com.hubspot.singularity.SingularityTaskHistory; import com.hubspot.singularity.SingularityTaskHistoryQuery; @@ -759,6 +760,15 @@ public List getRequestHistoryForRequest( @Parameter(required = true, description = "Request ID to look up") @PathParam( "requestId" ) String requestId, + @Parameter( + description = "Optionally match request histories created before" + ) @QueryParam("createdBefore") Optional createdBefore, + @Parameter( + description = "Optionally match request histories created after" + ) @QueryParam("createdAfter") Optional createdAfter, + @Parameter(description = "Sort direction") @QueryParam( + "orderDirection" + ) Optional orderDirection, @Parameter(description = "Maximum number of items to return") @QueryParam( "count" ) Integer count, @@ -777,9 +787,15 @@ public List getRequestHistoryForRequest( final Integer limitCount = getLimitCount(count); final Integer limitStart = getLimitStart(limitCount, page); + SingularityRequestHistoryQuery requestHistoryQuery = new SingularityRequestHistoryQuery( + requestId, + createdBefore, + createdAfter, + orderDirection + ); return requestHistoryHelper.getBlendedHistory( - requestId, + requestHistoryQuery, limitStart, limitCount, skipZk @@ -810,14 +826,20 @@ public SingularityPaginatedResponse getRequestHistory SingularityAuthorizationScope.READ ); - final Optional dataCount = requestHistoryHelper.getBlendedHistoryCount( + SingularityRequestHistoryQuery requestHistoryQuery = new SingularityRequestHistoryQuery( requestId, + Optional.empty(), + Optional.empty(), + Optional.empty() + ); + final Optional dataCount = requestHistoryHelper.getBlendedHistoryCount( + requestHistoryQuery, skipZk ); final Integer limitCount = getLimitCount(count); final Integer limitStart = getLimitStart(limitCount, page); final List data = requestHistoryHelper.getBlendedHistory( - requestId, + requestHistoryQuery, limitStart, limitCount, skipZk diff --git a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java index 28e9d6237b..731867f368 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java @@ -1252,6 +1252,8 @@ public void testMessage() { List history = historyManager.getRequestHistory( requestId, + Optional.empty(), + Optional.empty(), Optional.of(OrderDirection.DESC), 0, 100 @@ -1291,7 +1293,14 @@ public void testDuplicatePersist() { ); requestHistoryPersister.runActionOnPoll(); SingularityRequestHistory history = historyManager - .getRequestHistory(requestId, Optional.of(OrderDirection.DESC), 0, 100) + .getRequestHistory( + requestId, + Optional.empty(), + Optional.empty(), + Optional.of(OrderDirection.DESC), + 0, + 100 + ) .get(0); historyManager.saveRequestHistoryUpdate(history); // Should not throw exception @@ -1375,7 +1384,14 @@ public void testMigrateToJson() throws IOException { configuration.setSqlFallBackToBytesFields(true); SingularityRequestHistory requestHistoryBefore = historyManager - .getRequestHistory(request.getId(), Optional.empty(), 0, 1) + .getRequestHistory( + request.getId(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + 0, + 1 + ) .get(0); Assertions.assertNotNull(requestHistoryBefore); Assertions.assertEquals( @@ -1400,7 +1416,14 @@ public void testMigrateToJson() throws IOException { configuration.setSqlFallBackToBytesFields(false); SingularityRequestHistory requestHistoryAfter = historyManager - .getRequestHistory(request.getId(), Optional.empty(), 0, 1) + .getRequestHistory( + request.getId(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + 0, + 1 + ) .get(0); Assertions.assertNotNull(requestHistoryAfter); Assertions.assertEquals( diff --git a/SingularityService/src/test/java/com/hubspot/singularity/data/BlendedHistoryTest.java b/SingularityService/src/test/java/com/hubspot/singularity/data/BlendedHistoryTest.java index 56bc7a33fe..b47d77dbc4 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/data/BlendedHistoryTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/data/BlendedHistoryTest.java @@ -9,6 +9,7 @@ import com.hubspot.singularity.SingularityRequestBuilder; import com.hubspot.singularity.SingularityRequestHistory; import com.hubspot.singularity.SingularityRequestHistory.RequestHistoryType; +import com.hubspot.singularity.SingularityRequestHistoryQuery; import com.hubspot.singularity.config.SingularityConfiguration; import com.hubspot.singularity.data.history.HistoryManager; import com.hubspot.singularity.data.history.RequestHistoryHelper; @@ -36,6 +37,8 @@ private void mockRequestHistory( when( hm.getRequestHistory( ArgumentMatchers.anyString(), + ArgumentMatchers.>any(), + ArgumentMatchers.>any(), ArgumentMatchers.>any(), ArgumentMatchers.anyInt(), ArgumentMatchers.anyInt() @@ -65,6 +68,12 @@ private SingularityRequestHistory makeHistory(long createdAt, RequestHistoryType public void testBlendedRequestHistory() { HistoryManager hm = mock(HistoryManager.class); String rid = "rid"; + SingularityRequestHistoryQuery requestHistoryQuery = new SingularityRequestHistoryQuery( + rid, + Optional.empty(), + Optional.empty(), + Optional.empty() + ); request = new SingularityRequestBuilder(rid, RequestType.WORKER).build(); RequestHistoryHelper rhh = new RequestHistoryHelper( requestManager, @@ -74,7 +83,7 @@ public void testBlendedRequestHistory() { mockRequestHistory(hm, Collections.emptyList()); - Assertions.assertTrue(rhh.getBlendedHistory(rid, 0, 100).isEmpty()); + Assertions.assertTrue(rhh.getBlendedHistory(requestHistoryQuery, 0, 100).isEmpty()); Assertions.assertTrue(!rhh.getFirstHistory(rid).isPresent()); Assertions.assertTrue(!rhh.getLastHistory(rid).isPresent()); @@ -87,26 +96,30 @@ public void testBlendedRequestHistory() { ) ); - List history = rhh.getBlendedHistory(rid, 0, 5); + List history = rhh.getBlendedHistory( + requestHistoryQuery, + 0, + 5 + ); Assertions.assertTrue(history.size() == 3); saveHistory(100, RequestHistoryType.DELETED); saveHistory(120, RequestHistoryType.CREATED); - history = rhh.getBlendedHistory(rid, 0, 5); + history = rhh.getBlendedHistory(requestHistoryQuery, 0, 5); Assertions.assertTrue(history.size() == 5); Assertions.assertTrue(history.get(0).getCreatedAt() == 120); Assertions.assertTrue(history.get(4).getCreatedAt() == 50); - history = rhh.getBlendedHistory(rid, 1, 5); + history = rhh.getBlendedHistory(requestHistoryQuery, 1, 5); Assertions.assertTrue(history.size() == 4); Assertions.assertTrue(history.get(0).getCreatedAt() == 100); Assertions.assertTrue(history.get(3).getCreatedAt() == 50); - history = rhh.getBlendedHistory(rid, 2, 5); + history = rhh.getBlendedHistory(requestHistoryQuery, 2, 5); Assertions.assertTrue(history.size() == 3); Assertions.assertTrue(history.get(0).getCreatedAt() == 52); @@ -114,10 +127,10 @@ public void testBlendedRequestHistory() { mockRequestHistory(hm, Collections.emptyList()); - history = rhh.getBlendedHistory(rid, 3, 5); + history = rhh.getBlendedHistory(requestHistoryQuery, 3, 5); Assertions.assertTrue(history.isEmpty()); - history = rhh.getBlendedHistory(rid, 1, 5); + history = rhh.getBlendedHistory(requestHistoryQuery, 1, 5); Assertions.assertTrue(history.size() == 1); Assertions.assertTrue(history.get(0).getCreatedAt() == 100); From af175d7d81f246ab20cbf06d4c1ad12298580ce4 Mon Sep 17 00:00:00 2001 From: Faaya Fulas Date: Wed, 15 Jul 2020 18:28:13 -0400 Subject: [PATCH 2/4] Fix formatting --- .../SingularityRequestHistoryQuery.java | 2 +- .../singularity/client/SingularityClient.java | 28 +++++++++++-------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java index ab1811f6a3..e7fb021e98 100644 --- a/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java +++ b/SingularityBase/src/main/java/com/hubspot/singularity/SingularityRequestHistoryQuery.java @@ -84,7 +84,7 @@ public int compare(SingularityRequestHistory o1, SingularityRequestHistory o2) { @Override public String toString() { return ( - "SingularityTaskHistoryQuery{" + + "SingularityRequestHistoryQuery{" + "requestId=" + requestId + ", createdBefore=" + diff --git a/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java b/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java index 195ca2a42a..8745542d4f 100644 --- a/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java +++ b/SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java @@ -1682,14 +1682,14 @@ public void deleteSlave(String slaveId) { * A list of {@link SingularityRequestHistory} */ public Collection getHistoryForRequest( - String requestId, - Optional createdBefore, - Optional createdAfter, - Optional count, - Optional page + String requestId, + Optional createdBefore, + Optional createdAfter, + Optional count, + Optional page ) { final Function requestUri = host -> - String.format(REQUEST_HISTORY_FORMAT, getApiBase(host), requestId); + String.format(REQUEST_HISTORY_FORMAT, getApiBase(host), requestId); Optional> maybeQueryParams = Optional.empty(); @@ -1717,10 +1717,10 @@ public Collection getHistoryForRequest( } return getCollectionWithParams( - requestUri, - "request history", - maybeQueryParams, - REQUEST_HISTORY_COLLECTION + requestUri, + "request history", + maybeQueryParams, + REQUEST_HISTORY_COLLECTION ); } @@ -1729,7 +1729,13 @@ public Collection getHistoryForRequest( Optional count, Optional page ) { - return getHistoryForRequest(requestId, Optional.empty(), Optional.empty(), count, page); + return getHistoryForRequest( + requestId, + Optional.empty(), + Optional.empty(), + count, + page + ); } // From 6aaa8f24e71055c63d3c2e00ed92ace97176bc11 Mon Sep 17 00:00:00 2001 From: Faaya Fulas Date: Thu, 16 Jul 2020 12:05:31 -0400 Subject: [PATCH 3/4] Add tests --- .../data/history/MySQLHistoryJDBI.java | 12 --- .../data/history/PostgresHistoryJDBI.java | 12 --- .../singularity/SingularityHistoryTest.java | 86 +++++++++++++++++++ 3 files changed, 86 insertions(+), 24 deletions(-) diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java index aab8cec3da..d500e8dcf4 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/MySQLHistoryJDBI.java @@ -2,7 +2,6 @@ import com.hubspot.singularity.SingularityDeployHistory; import com.hubspot.singularity.SingularityRequest; -import com.hubspot.singularity.SingularityRequestHistory; import com.hubspot.singularity.SingularityTaskHistory; import com.hubspot.singularity.data.history.SingularityMappers.SingularityRequestIdCount; import java.util.Date; @@ -10,7 +9,6 @@ import org.jdbi.v3.json.Json; import org.jdbi.v3.sqlobject.SingleValue; import org.jdbi.v3.sqlobject.customizer.Bind; -import org.jdbi.v3.sqlobject.customizer.Define; import org.jdbi.v3.sqlobject.statement.SqlQuery; import org.jdbi.v3.sqlobject.statement.SqlUpdate; @@ -93,16 +91,6 @@ List getDeployHistoryForRequest( @SqlQuery("SELECT COUNT(*) FROM deployHistory WHERE requestId = :requestId") int getDeployHistoryForRequestCount(@Bind("requestId") String requestId); - // @SqlQuery( - // "SELECT json, request, createdAt, requestState, user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt LIMIT :limitStart, :limitCount" - // ) - // List getRequestHistory( - // @Bind("requestId") String requestId, - // @Define("orderDirection") String orderDirection, - // @Bind("limitStart") Integer limitStart, - // @Bind("limitCount") Integer limitCount - // ); - @SqlQuery("SELECT COUNT(*) FROM requestHistory WHERE requestId = :requestId") int getRequestHistoryCount(@Bind("requestId") String requestId); diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java index ddc8c4c5d5..775141f0d5 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/PostgresHistoryJDBI.java @@ -2,7 +2,6 @@ import com.hubspot.singularity.SingularityDeployHistory; import com.hubspot.singularity.SingularityRequest; -import com.hubspot.singularity.SingularityRequestHistory; import com.hubspot.singularity.SingularityTaskHistory; import com.hubspot.singularity.data.history.SingularityMappers.SingularityRequestIdCount; import java.util.Date; @@ -10,7 +9,6 @@ import org.jdbi.v3.json.Json; import org.jdbi.v3.sqlobject.SingleValue; import org.jdbi.v3.sqlobject.customizer.Bind; -import org.jdbi.v3.sqlobject.customizer.Define; import org.jdbi.v3.sqlobject.statement.SqlQuery; import org.jdbi.v3.sqlobject.statement.SqlUpdate; @@ -93,16 +91,6 @@ List getDeployHistoryForRequest( @SqlQuery("SELECT COUNT(*) FROM deployHistory WHERE requestId = :requestId") int getDeployHistoryForRequestCount(@Bind("requestId") String requestId); - // @SqlQuery( - // "SELECT json, request, createdAt, requestState, f_user, message FROM requestHistory WHERE requestId = :requestId ORDER BY createdAt OFFSET :limitStart LIMIT :limitCount" - // ) - // List getRequestHistory( - // @Bind("requestId") String requestId, - // @Define("orderDirection") String orderDirection, - // @Bind("limitStart") Integer limitStart, - // @Bind("limitCount") Integer limitCount - // ); - @SqlQuery("SELECT COUNT(*) FROM requestHistory WHERE requestId = :requestId") int getRequestHistoryCount(@Bind("requestId") String requestId); diff --git a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java index 731867f368..d8417e16f8 100644 --- a/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java +++ b/SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java @@ -1274,6 +1274,92 @@ public void testMessage() { } } + @Test + public void testGetRequestHistoryWithCreatedAtFilters() { + SingularityRequest request = new SingularityRequestBuilder( + requestId, + RequestType.ON_DEMAND + ) + .build(); + SingularityRequestHistory firstRequestHistory = new SingularityRequestHistory( + 0L, + Optional.empty(), + RequestHistoryType.CREATED, + request, + Optional.empty() + ); + + SingularityRequestHistory secondRequestHistory = new SingularityRequestHistory( + 10L, + Optional.empty(), + RequestHistoryType.BOUNCED, + request, + Optional.empty() + ); + + SingularityRequestHistory thirdRequestHistory = new SingularityRequestHistory( + 20L, + Optional.empty(), + RequestHistoryType.SCALED, + request, + Optional.empty() + ); + + historyManager.saveRequestHistoryUpdate(firstRequestHistory); + historyManager.saveRequestHistoryUpdate(secondRequestHistory); + historyManager.saveRequestHistoryUpdate(thirdRequestHistory); + + List firstHistory = historyManager.getRequestHistory( + requestId, + Optional.of(10L), //createdBefore + Optional.empty(), //createdAfter + Optional.of(OrderDirection.DESC), + 0, + 100 + ); + + Assertions.assertEquals(1, firstHistory.size()); + Assertions.assertTrue(firstHistory.get(0).getCreatedAt() == 0); + + List secondHistory = historyManager.getRequestHistory( + requestId, + Optional.of(20L), //createdBefore + Optional.of(0L), //createdAfter + Optional.of(OrderDirection.DESC), + 0, + 100 + ); + + Assertions.assertEquals(1, secondHistory.size()); + Assertions.assertTrue(secondHistory.get(0).getCreatedAt() == 10); + + List thirdHistory = historyManager.getRequestHistory( + requestId, + Optional.empty(), //createdBefore + Optional.of(10L), //createdAfter + Optional.of(OrderDirection.DESC), + 0, + 100 + ); + + Assertions.assertEquals(1, thirdHistory.size()); + Assertions.assertTrue(thirdHistory.get(0).getCreatedAt() == 20); + + //test ascending order + List historyAscending = historyManager.getRequestHistory( + requestId, + Optional.empty(), //createdBefore + Optional.empty(), //createdAfter + Optional.of(OrderDirection.ASC), + 0, + 100 + ); + + Assertions.assertEquals(3, historyAscending.size()); + Assertions.assertTrue(historyAscending.get(0).getCreatedAt() == 0); + Assertions.assertTrue(historyAscending.get(2).getCreatedAt() == 20); + } + @Test public void testDuplicatePersist() { initRequest(); From 1fa773ba83d2f463d1d3b07b8ba69a7627fae22f Mon Sep 17 00:00:00 2001 From: Faaya Fulas Date: Thu, 16 Jul 2020 16:11:12 -0400 Subject: [PATCH 4/4] Remove unused import --- .../hubspot/singularity/data/history/AbstractHistoryJDBI.java | 1 - 1 file changed, 1 deletion(-) diff --git a/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java b/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java index f06dd7b2e8..3ec058e6e4 100644 --- a/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java +++ b/SingularityService/src/main/java/com/hubspot/singularity/data/history/AbstractHistoryJDBI.java @@ -4,7 +4,6 @@ import com.hubspot.singularity.OrderDirection; import com.hubspot.singularity.SingularityRequestHistory; import com.hubspot.singularity.SingularityTaskIdHistory; -import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.List;