small fixes, always persist request history

SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java

@@ -310,7 +310,7 @@ public Optional<SingularityRequestWithState> getRequest(String requestId) {
     return getData(getRequestPath(requestId), requestTranscoder);
   }
 
-  public void deleteRequest(SingularityRequest request, Optional<String> user, Optional<String> actionId, Optional<String> message) {
+  public SingularityDeleteResult deleteRequest(SingularityRequest request, Optional<String> user, Optional<String> actionId, Optional<String> message) {
     final long now = System.currentTimeMillis();
 
     // delete it no matter if the delete request already exists.
@@ -319,7 +319,11 @@ public void deleteRequest(SingularityRequest request, Optional<String> user, Opt
 
     saveHistory(new SingularityRequestHistory(now, user, RequestHistoryType.DELETED, request, message));
 
-    delete(getRequestPath(request.getId()));
+    SingularityDeleteResult deleteResult = delete(getRequestPath(request.getId()));
+
+    LOG.info("Request {} deleted ({}) by {} - {}", request.getId(), deleteResult, user, message);
+
+    return deleteResult;
   }
 
   public List<SingularityRequestLbCleanup> getLbCleanupRequests() {

SingularityService/src/main/java/com/hubspot/singularity/data/history/SingularityRequestHistoryPersister.java

@@ -1,15 +1,13 @@
 package com.hubspot.singularity.data.history;
 
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import javax.inject.Singleton;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.collect.Sets;
 import com.google.inject.Inject;
 import com.hubspot.mesos.JavaUtils;
 import com.hubspot.singularity.SingularityDeleteResult;
@@ -70,16 +68,11 @@ public void runActionOnPoll() {
     final long start = System.currentTimeMillis();
 
     final List<String> requestIdsWithHistory = requestManager.getRequestIdsWithHistory();
-    final Set<String> requestIds = Sets.newHashSet(requestManager.getAllRequestIds());
 
     int numHistoryTransferred = 0;
     int numRequests = 0;
 
     for (String requestId : requestIdsWithHistory) {
-      if (!requestIds.contains(requestId)) {
-        continue;
-      }
-
       numRequests++;
 
       List<SingularityRequestHistory> historyForRequestId = requestManager.getRequestHistory(requestId);

SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java

@@ -233,7 +233,9 @@ public SingularityPendingRequestParent scheduleImmediately(@ApiParam("The reques
       commandLineArgs = runNowRequest.get().getCommandLineArgs();
     }
 
-    checkBadRequest(!runId.isPresent() || runId.get().length() < 100, "runId must be less than 100 characters. RunId %s has %s characters", runId.get(), runId.get().length());
+    if (runId.isPresent() && runId.get().length() > 100) {
+      throw badRequest("runId must be less than 100 characters. RunId %s has %s characters", runId.get(), runId.get().length());
+    }
 
     if (!runId.isPresent()) {
       runId = Optional.of(UUID.randomUUID().toString());

SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryPurgerTest.java → SingularityService/src/test/java/com/hubspot/singularity/SingularityHistoryTest.java

@@ -21,11 +21,16 @@
 import com.google.common.base.Optional;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.hubspot.singularity.SingularityRequestHistory.RequestHistoryType;
+import com.hubspot.singularity.api.SingularityDeleteRequestRequest;
 import com.hubspot.singularity.api.SingularityRunNowRequest;
+import com.hubspot.singularity.api.SingularityScaleRequest;
 import com.hubspot.singularity.config.HistoryPurgingConfiguration;
 import com.hubspot.singularity.data.TaskManager;
 import com.hubspot.singularity.data.history.HistoryManager;
+import com.hubspot.singularity.data.history.HistoryManager.OrderDirection;
 import com.hubspot.singularity.data.history.SingularityHistoryPurger;
+import com.hubspot.singularity.data.history.SingularityRequestHistoryPersister;
 import com.hubspot.singularity.data.history.SingularityTaskHistoryPersister;
 import com.hubspot.singularity.data.history.TaskHistoryHelper;
 
@@ -35,7 +40,7 @@
 import liquibase.database.jvm.JdbcConnection;
 import liquibase.resource.FileSystemResourceAccessor;
 
-public class SingularityHistoryPurgerTest extends SingularitySchedulerTestBase {
+public class SingularityHistoryTest extends SingularitySchedulerTestBase {
 
   @Inject
   protected Provider<DBI> dbiProvider;
@@ -46,10 +51,13 @@ public class SingularityHistoryPurgerTest extends SingularitySchedulerTestBase {
   @Inject
   protected SingularityTaskHistoryPersister taskHistoryPersister;
 
+  @Inject
+  protected SingularityRequestHistoryPersister requestHistoryPersister;
+
   @Inject
   protected SingularityTestAuthenticator testAuthenticator;
 
-  public SingularityHistoryPurgerTest() {
+  public SingularityHistoryTest() {
     super(true);
   }
 
@@ -223,4 +231,38 @@ public void testPersisterRaceCondition() {
     // assert that the history works, but more importantly, that we don't NPE
     Assert.assertEquals(1, taskHistoryHelperWithMockedTaskManager.getBlendedHistory(requestId, 0, 5).size());
   }
+
+  @Test
+  public void testMessage() {
+    initRequest();
+
+    String msg = null;
+    for (int i = 0; i < 300; i++) {
+      msg = msg + i;
+    }
+
+    requestResource.scale(requestId, new SingularityScaleRequest(Optional.of(1), Optional.<Long> absent(), Optional.<Boolean> absent(), Optional.<String> absent(), Optional.of(msg)));
+    requestResource.deleteRequest(requestId, Optional.of(new SingularityDeleteRequestRequest(Optional.of("a msg"), Optional.<String> absent())));
+
+    cleaner.drainCleanupQueue();
+
+    requestHistoryPersister.runActionOnPoll();
+
+    List<SingularityRequestHistory> history = historyManager.getRequestHistory(requestId, Optional.of(OrderDirection.DESC), 0, 100);
+
+    Assert.assertEquals(3, history.size());
+
+    for (SingularityRequestHistory historyItem : history) {
+      if (historyItem.getEventType() == RequestHistoryType.DELETED) {
+        Assert.assertEquals("a msg", historyItem.getMessage().get());
+      } else if (historyItem.getEventType() == RequestHistoryType.UPDATED) {
+        Assert.assertEquals(280, historyItem.getMessage().get().length());
+      } else {
+        Assert.assertTrue(!historyItem.getMessage().isPresent());
+      }
+    }
+
+  }
+
+
 }