Skip to content

Commit

Permalink
Repond to PR comments
Browse files Browse the repository at this point in the history 
Respond to PR comments. In particular
- Style change: reduce the number of line breaks in method calls
- Don't validate task count on enqueue; defer it to the deploy is going to be run
- Guaranteed that _something_ will be deployed, even when a task couldn't
  be run.
  • Loading branch information
@PtrTeixeira
PtrTeixeira committed May 23, 2017
1 parent 325ba75 commit c323f8f
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 169 deletions.
24 changes: 4 additions & 20 deletions SingularityService/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
View file
Expand Up @@ -365,12 +365,7 @@ public SingularityDeploy checkDeploy(SingularityRequest request,
checkBadRequest(deployHistoryHelper.isDeployIdAvailable(request.getId(), deployId), "Can not deploy a deploy that has already been deployed"); checkBadRequest(deployHistoryHelper.isDeployIdAvailable(request.getId(), deployId), "Can not deploy a deploy that has already been deployed");


if (deploy.getRunImmediately().isPresent()) { if (deploy.getRunImmediately().isPresent()) {
deploy = checkImmediateRunDeploy( deploy = checkImmediateRunDeploy(request, deploy, deploy.getRunImmediately().get(), activeTasks, pendingTasks);
request,
deploy,
deploy.getRunImmediately().get(),
activeTasks,
pendingTasks);
} }


if (request.isDeployable()) { if (request.isDeployable()) {
Expand All @@ -389,20 +384,9 @@ private SingularityDeploy checkImmediateRunDeploy(SingularityRequest request,
throw badRequest("Can not request an immediate run of a non-scheduled / always running request (%s)", request); throw badRequest("Can not request an immediate run of a non-scheduled / always running request (%s)", request);
} }


boolean canRunImmediately = return deploy.toBuilder()
(request.isScheduled() && activeTasks.isEmpty()) .setRunImmediately(Optional.of(fillRunNowRequest(Optional.of(runNowRequest))))
|| (request.isOneOff() && ! request.getInstances().isPresent()) .build();
|| (request.isOneOff() && request.getInstances().get() > activeTasks.size() + pendingTasks.size());

if (canRunImmediately) {
return deploy.toBuilder()
.setRunImmediately(Optional.of(fillRunNowRequest(Optional.of(runNowRequest))))
.build();
} else {
return deploy.toBuilder()
.setRunImmediately(Optional.absent())
.build();
}
} }


public SingularityPendingRequest checkRunNowRequest(String deployId, public SingularityPendingRequest checkRunNowRequest(String deployId,
Expand Down
150 changes: 41 additions & 109 deletions ...rityService/src/main/java/com/hubspot/singularity/scheduler/SingularityDeployChecker.java
View file
Expand Up @@ -269,21 +269,45 @@ private void finishDeploy(SingularityRequestWithState requestWithState, Optional
} }


if (deploy.isPresent() && deploy.get().getRunImmediately().isPresent()) { if (deploy.isPresent() && deploy.get().getRunImmediately().isPresent()) {
Optional<SingularityPendingRequest> maybePendingRequest = buildPendingRequest(request, String requestId = deploy.get().getRequestId();
pendingDeploy, String deployId = deploy.get().getId();
deployResult, long timestamp = deployResult.getTimestamp();
deploy.get().getRunImmediately()); SingularityRunNowRequest runNowRequest = deploy.get().getRunImmediately().get();
if (maybePendingRequest.isPresent()) { Optional<String> runId = runNowRequest.getRunId().or(Optional.of(UUID.randomUUID().toString()));
requestManager.addToPendingQueue(maybePendingRequest.get()); Optional<String> message = runNowRequest.getMessage()
.or(pendingDeploy.getDeployMarker().getMessage());
Optional<String> user = pendingDeploy.getDeployMarker().getUser();
Optional<List<String>> commandLineArgs = runNowRequest.getCommandLineArgs();
Optional<Boolean> skipHealthChecks = runNowRequest.getSkipHealthchecks().or(request.getSkipHealthchecks());
Optional<Resources> resources = runNowRequest.getResources();
List<SingularityTaskId> activeTasks = taskManager.getActiveTaskIdsForRequest(requestId);
List<SingularityPendingTaskId> pendingTasks = taskManager.getPendingTaskIdsForRequest(requestId);

if (request.isScheduled()) {
if (activeTasks.isEmpty()) {
PendingType pendingType = canceledOr(deployResult.getDeployState(), PendingType.IMMEDIATE);
requestManager.addToPendingQueue(new SingularityPendingRequest(requestId, deployId, timestamp, user, pendingType, commandLineArgs, runId, skipHealthChecks, message, Optional.absent(), resources));
} else {
// Don't run scheduled task over a running task. Will be picked up on the next run.
PendingType pendingType = canceledOr(deployResult.getDeployState(), PendingType.NEW_DEPLOY);
requestManager.addToPendingQueue(new SingularityPendingRequest(requestId, deployId, timestamp, user, pendingType, commandLineArgs, runId, skipHealthChecks, message, Optional.absent(), resources));
}
} else if (!request.isLongRunning()) {
if (request.getInstances().isPresent()
&& (activeTasks.size() + pendingTasks.size() < request.getInstances().get())) {
PendingType pendingType = canceledOr(deployResult.getDeployState(), PendingType.ONEOFF);
requestManager.addToPendingQueue(new SingularityPendingRequest(requestId, deployId, timestamp, user, pendingType, commandLineArgs, runId, skipHealthChecks, message, Optional.absent(), resources));
} else {
// Don't run one-off / on-demand task when already at instance count cap
PendingType pendingType = canceledOr(deployResult.getDeployState(), PendingType.NEW_DEPLOY);
requestManager.addToPendingQueue(new SingularityPendingRequest(requestId, deployId, timestamp, user, pendingType, commandLineArgs, runId, skipHealthChecks, message, Optional.absent(), resources));
}
} }
} else if (!request.isDeployable() && !request.isOneOff()) { } else if (!request.isDeployable() && !request.isOneOff()) {
Optional<SingularityPendingRequest> maybePendingRequest = buildPendingRequest(request, PendingType pendingType = canceledOr(deployResult.getDeployState(), PendingType.NEW_DEPLOY);
pendingDeploy, requestManager.addToPendingQueue(new SingularityPendingRequest(request.getId(), pendingDeploy.getDeployMarker().getDeployId(),
deployResult, deployResult.getTimestamp(), pendingDeploy.getDeployMarker().getUser(), pendingType,
Optional.absent()); deploy.isPresent() ? deploy.get().getSkipHealthchecksOnDeploy() : Optional.absent(), pendingDeploy.getDeployMarker().getMessage()));
if (maybePendingRequest.isPresent()) {
requestManager.addToPendingQueue(maybePendingRequest.get());
}
} }


if (!request.isDeployable() && !request.isOneOff()) { if (!request.isDeployable() && !request.isOneOff()) {
Expand Down Expand Up @@ -335,103 +359,11 @@ private void finishDeploy(SingularityRequestWithState requestWithState, Optional
removePendingDeploy(pendingDeploy); removePendingDeploy(pendingDeploy);
} }


private Optional<SingularityPendingRequest> buildPendingRequest(SingularityRequest request, private PendingType canceledOr(DeployState deployState, PendingType pendingType) {
SingularityPendingDeploy pendingDeploy, if (deployState == DeployState.CANCELED) {
SingularityDeployResult deployResult, return PendingType.DEPLOY_CANCELLED;
Optional<SingularityRunNowRequest> maybeRunNowRequest) {
String requestId = request.getId();
String deployId = pendingDeploy.getDeployMarker().getDeployId();
Optional<String> user = pendingDeploy.getDeployMarker().getUser();
long timestamp = deployResult.getTimestamp();
PendingType pendingType;
Optional<Boolean> skipHealthChecks = request.getSkipHealthchecks();
Optional<String> message = pendingDeploy.getDeployMarker().getMessage();
Optional<List<String>> commandLineArgs;
Optional<String> runId;
Optional<Resources> resources;
List<SingularityTaskId> activeTasks = taskManager.getActiveTaskIdsForRequest(requestId);
List<SingularityPendingTaskId> pendingTasks = taskManager.getPendingTaskIdsForRequest(requestId);

SingularityPendingRequest pendingRequest;

if (request.isScheduled()
&& maybeRunNowRequest.isPresent()
&& activeTasks.isEmpty()) {
SingularityRunNowRequest runNowRequest = maybeRunNowRequest.get();
runId = runNowRequest.getRunId().or(Optional.of(UUID.randomUUID().toString()));
message = runNowRequest.getMessage()
.or(message);
commandLineArgs = runNowRequest.getCommandLineArgs();
skipHealthChecks = runNowRequest.getSkipHealthchecks().or(skipHealthChecks);
pendingType = PendingType.IMMEDIATE;
resources = runNowRequest.getResources();
pendingRequest = new SingularityPendingRequest(
requestId,
deployId,
timestamp,
user,
pendingType,
commandLineArgs,
runId,
skipHealthChecks,
message,
Optional.absent(),
resources);
return Optional.of(pendingRequest);
} else if (request.isScheduled()) {
// There is already a running task for this request. Don't attempt to run now, just redeploy
pendingType = deployResult.getDeployState() == DeployState.CANCELED
? PendingType.DEPLOY_CANCELLED
: PendingType.NEW_DEPLOY;
pendingRequest = new SingularityPendingRequest(
requestId,
deployId,
timestamp,
user,
pendingType,
skipHealthChecks,
message);
return Optional.of(pendingRequest);
} else if (!request.isAlwaysRunning()
&& maybeRunNowRequest.isPresent()
&& request.getInstances().isPresent()
&& (activeTasks.size() + pendingTasks.size() < request.getInstances().get())) {
SingularityRunNowRequest runNowRequest = maybeRunNowRequest.get();
pendingType = PendingType.ONEOFF;
runId = runNowRequest.getRunId().or(Optional.of(UUID.randomUUID().toString()));
message = runNowRequest.getMessage()
.or(message);
commandLineArgs = runNowRequest.getCommandLineArgs();
skipHealthChecks = runNowRequest.getSkipHealthchecks().or(skipHealthChecks);
resources = runNowRequest.getResources();
pendingRequest = new SingularityPendingRequest(
requestId,
deployId,
timestamp,
user,
pendingType,
commandLineArgs,
runId,
skipHealthChecks,
message,
Optional.absent(),
resources);
return Optional.of(pendingRequest);
} else if (!request.isOneOff()) {
pendingType = deployResult.getDeployState() == DeployState.CANCELED
? PendingType.DEPLOY_CANCELLED
: PendingType.NEW_DEPLOY;
pendingRequest = new SingularityPendingRequest(
requestId,
deployId,
timestamp,
user,
pendingType,
skipHealthChecks,
message);
return Optional.of(pendingRequest);
} else { } else {
return Optional.absent(); return pendingType;
} }
} }


Expand Down
40 changes: 0 additions & 40 deletions SingularityService/src/test/java/com/hubspot/singularity/data/ValidatorTest.java
View file
Expand Up @@ -216,46 +216,6 @@ public void whenDeployNotOneOffOrScheduledItForbidsRunImmediately() {
validator.checkDeploy(request, deploy, Collections.emptyList(), Collections.emptyList()); validator.checkDeploy(request, deploy, Collections.emptyList(), Collections.emptyList());
} }


@Test
public void whenRunNowSetAndScheduledTaskAndAlreadyRunningItWillNotRunImmediately() {
String requestId = "request";
String deployID = "deploy";
SingularityRequest request = new SingularityRequestBuilder(requestId, RequestType.SCHEDULED)
.build();
Optional<SingularityRunNowRequest> runNowRequest = Optional.of(runNowRequest());
SingularityTaskId activeTask = new SingularityTaskId(requestId, deployID, 0, 1, "host", "rack");

SingularityDeploy deploy = SingularityDeploy.newBuilder(requestId, deployID)
.setCommand(Optional.of("printenv"))
.setRunImmediately(runNowRequest)
.build();

SingularityDeploy result = validator.checkDeploy(request, deploy, Collections.singletonList(activeTask()), Collections.emptyList());

Assert.assertFalse("Run immediately is no longer set", result.getRunImmediately().isPresent());
}

@Test
public void whenRunNowSetAndOneOffAndTasksAlreadyRunningItWillNotRunImmediately() {
String requestId = "request";
String deployID = "deploy";
SingularityRequest request = new SingularityRequestBuilder(requestId, RequestType.ON_DEMAND)
.setInstances(Optional.of(2))
.build();
Optional<SingularityRunNowRequest> runNowRequest = Optional.of(runNowRequest());
SingularityTaskId activeTask1 = new SingularityTaskId(requestId, deployID, 0, 1, "host", "rack");
SingularityTaskId activeTask2 = new SingularityTaskId(requestId, deployID, 0, 1, "host", "rack");

SingularityDeploy deploy = SingularityDeploy.newBuilder(requestId, deployID)
.setCommand(Optional.of("printenv"))
.setRunImmediately(runNowRequest)
.build();

SingularityDeploy result = validator.checkDeploy(request, deploy, Arrays.asList(activeTask1, activeTask2), Collections.emptyList());

Assert.assertFalse("Run immediately is no longer set", result.getRunImmediately().isPresent());
}

private SingularityTaskId activeTask() { private SingularityTaskId activeTask() {
return new SingularityTaskId( return new SingularityTaskId(
"requestId", "requestId",
Expand Down
2 changes: 2 additions & 0 deletions ...rc/test/java/com/hubspot/singularity/scheduler/SingularityOfferPerformanceTestRunner.java
View file
Expand Up @@ -6,6 +6,7 @@
import java.util.Random; import java.util.Random;


import org.apache.mesos.Protos.Offer; import org.apache.mesos.Protos.Offer;
import org.junit.Ignore;
import org.junit.Test; import org.junit.Test;


import com.hubspot.mesos.JavaUtils; import com.hubspot.mesos.JavaUtils;
Expand All @@ -17,6 +18,7 @@ public SingularityOfferPerformanceTestRunner() {
} }


@Test @Test
@Ignore
public void testOfferCache() { public void testOfferCache() {
long start = System.currentTimeMillis(); long start = System.currentTimeMillis();


Expand Down

0 comments on commit c323f8f

Please sign in to comment.