Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guarantee durationMillis is present in getExpiringBounce response #1197

Merged
merged 13 commits into from
Aug 25, 2016
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ public static SingularityBounceRequest defaultRequest() {
return new SingularityBounceRequest(Optional.<Boolean>absent(), Optional.<Boolean>absent(), Optional.<Long>absent(), Optional.of(UUID.randomUUID().toString()), Optional.<String>absent());
}

public SingularityBounceRequestBuilder toBuilder() {
return new SingularityBounceRequestBuilder()
.setIncremental(incremental)
.setSkipHealthchecks(skipHealthchecks)
.setDurationMillis(getDurationMillis())
.setActionId(getActionId())
.setMessage(getMessage());
}

@ApiModelProperty(required=false, value="If present and set to true, old tasks will be killed as soon as replacement tasks are available, instead of waiting for all replacement tasks to be healthy")
public Optional<Boolean> getIncremental() {
return incremental;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package com.hubspot.singularity.api;

import com.google.common.base.Optional;

public class SingularityBounceRequestBuilder {

private Optional<Boolean> incremental;
private Optional<Boolean> skipHealthchecks;

private Optional<Long> durationMillis;
private Optional<String> actionId;
private Optional<String> message;

public SingularityBounceRequestBuilder() {
this.incremental = Optional.absent();
this.skipHealthchecks = Optional.absent();

this.durationMillis = Optional.absent();
this.actionId = Optional.absent();
this.message = Optional.absent();
}

public SingularityBounceRequest build() {
return new SingularityBounceRequest(incremental, skipHealthchecks, durationMillis, actionId, message);
}

public Optional<Long> getDurationMillis() {
return durationMillis;
}

public SingularityBounceRequestBuilder setDurationMillis(Optional<Long> durationMillis) {
this.durationMillis = durationMillis;
return this;
}

public Optional<String> getActionId() {
return actionId;
}

public SingularityBounceRequestBuilder setActionId(Optional<String> actionId) {
this.actionId = actionId;
return this;
}

public Optional<String> getMessage() {
return message;
}

public SingularityBounceRequestBuilder setMessage(Optional<String> message) {
this.message = message;
return this;
}

public Optional<Boolean> getIncremental() {
return incremental;
}

public SingularityBounceRequestBuilder setIncremental(Optional<Boolean> incremental) {
this.incremental = incremental;
return this;
}

public Optional<Boolean> getSkipHealthchecks() {
return skipHealthchecks;
}

public SingularityBounceRequestBuilder setSkipHealthchecks(Optional<Boolean> skipHealthchecks) {
this.skipHealthchecks = skipHealthchecks;
return this;
}

@Override
public String toString() {
return "SingularityBounceRequestBuilder{" +
"durationMillis='" + durationMillis + '\'' +
", actionId='" + actionId + '\'' +
", message=" + message + '\'' +
", incremental=" + incremental + '\'' +
", skipHealthchecks=" + skipHealthchecks + '\'' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you might have changed this already, but the last three parts are missing the first apostrophe ", incremental=" => ", incremental='"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

"}";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class RequestManager extends CuratorAsyncManager {
private final Transcoder<SingularityRequestLbCleanup> requestLbCleanupTranscoder;

private final SingularityEventListener singularityEventListener;
private final SingularityConfiguration singularityConfiguration;

private static final String REQUEST_ROOT = "/requests";

Expand Down Expand Up @@ -80,14 +81,16 @@ public class RequestManager extends CuratorAsyncManager {
public RequestManager(CuratorFramework curator, SingularityConfiguration configuration, MetricRegistry metricRegistry, SingularityEventListener singularityEventListener,
Transcoder<SingularityRequestCleanup> requestCleanupTranscoder, Transcoder<SingularityRequestWithState> requestTranscoder, Transcoder<SingularityRequestLbCleanup> requestLbCleanupTranscoder,
Transcoder<SingularityPendingRequest> pendingRequestTranscoder, Transcoder<SingularityRequestHistory> requestHistoryTranscoder, Transcoder<SingularityExpiringBounce> expiringBounceTranscoder,
Transcoder<SingularityExpiringScale> expiringScaleTranscoder, Transcoder<SingularityExpiringPause> expiringPauseTranscoder, Transcoder<SingularityExpiringSkipHealthchecks> expiringSkipHealthchecksTranscoder) {
Transcoder<SingularityExpiringScale> expiringScaleTranscoder, Transcoder<SingularityExpiringPause> expiringPauseTranscoder, Transcoder<SingularityExpiringSkipHealthchecks> expiringSkipHealthchecksTranscoder,
SingularityConfiguration singularityConfiguration) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

singularityConfiguration is not in use

super(curator, configuration, metricRegistry);
this.requestTranscoder = requestTranscoder;
this.requestCleanupTranscoder = requestCleanupTranscoder;
this.pendingRequestTranscoder = pendingRequestTranscoder;
this.requestHistoryTranscoder = requestHistoryTranscoder;
this.singularityEventListener = singularityEventListener;
this.requestLbCleanupTranscoder = requestLbCleanupTranscoder;
this.singularityConfiguration = singularityConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't look like singularityConfiguration is in use in this class anymore, can you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sure thing. The perils of moving methods around.


this.expiringTranscoderMap = ImmutableMap.of(
SingularityExpiringBounce.class, expiringBounceTranscoder,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.List;
import java.util.TimeZone;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;

import javax.inject.Singleton;
Expand All @@ -30,7 +31,6 @@
import com.hubspot.mesos.SingularityContainerInfo;
import com.hubspot.mesos.SingularityContainerType;
import com.hubspot.mesos.SingularityDockerInfo;
import com.hubspot.mesos.SingularityDockerNetworkType;
import com.hubspot.mesos.SingularityDockerPortMapping;
import com.hubspot.mesos.SingularityPortMappingType;
import com.hubspot.mesos.SingularityVolume;
Expand All @@ -39,6 +39,7 @@
import com.hubspot.singularity.SingularityDeployBuilder;
import com.hubspot.singularity.SingularityRequest;
import com.hubspot.singularity.SingularityWebhook;
import com.hubspot.singularity.api.SingularityBounceRequest;
import com.hubspot.singularity.config.SingularityConfiguration;
import com.hubspot.singularity.data.history.DeployHistoryHelper;

Expand All @@ -48,6 +49,7 @@ public class SingularityValidator {
private static final List<Character> DEPLOY_ID_ILLEGAL_CHARACTERS = Arrays.asList('@', '-', '\\', '/', '*', '?', '%', ' ', '[', ']', '#', '$'); // Characters that make Mesos or URL bars sad
private static final List<Character> REQUEST_ID_ILLEGAL_CHARACTERS = Arrays.asList('@', '\\', '/', '*', '?', '%', ' ', '[', ']', '#', '$'); // Characters that make Mesos or URL bars sad

private final SingularityConfiguration configuration;
private final int maxDeployIdSize;
private final int maxRequestIdSize;
private final int maxCpusPerRequest;
Expand All @@ -66,6 +68,8 @@ public class SingularityValidator {

@Inject
public SingularityValidator(SingularityConfiguration configuration, DeployHistoryHelper deployHistoryHelper, RequestManager requestManager) {
this.configuration = configuration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd mimic what we're doing everywhere else in this class and just have a variable for the specific configuration value you need (in this case, defaultBounceExpirationMinutes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And define this variable in SingularityConfiguration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variable is already in SingularityConfiguration, it's accessed via configuration.getDefaultBounceExpirationMinutes()


this.maxDeployIdSize = configuration.getMaxDeployIdSize();
this.maxRequestIdSize = configuration.getMaxRequestIdSize();
this.allowRequestsWithoutOwners = configuration.isAllowRequestsWithoutOwners();
Expand Down Expand Up @@ -436,4 +440,15 @@ private boolean isValidInteger(String strValue) {
return false;
}
}

public SingularityBounceRequest validateBounceRequest(SingularityBounceRequest defaultBounceRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a nitpick, but all the other validation methods use check instead of validate in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix!

if (defaultBounceRequest.getDurationMillis().isPresent()) {
return defaultBounceRequest;
}
final long durationMillis = TimeUnit.MINUTES.toMillis(configuration.getDefaultBounceExpirationMinutes());
return defaultBounceRequest
.toBuilder()
.setDurationMillis(Optional.of(durationMillis))
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,10 @@ public SingularityRequestParent bounce(@ApiParam("The request ID to bounce") @Pa

requestManager.bounce(requestWithState.getRequest(), System.currentTimeMillis(), JavaUtils.getUserEmail(user), message);

final SingularityBounceRequest validatedBounceRequest = validator.validateBounceRequest(bounceRequest.or(SingularityBounceRequest.defaultRequest()));

requestManager.saveExpiringObject(new SingularityExpiringBounce(requestId, deployId, JavaUtils.getUserEmail(user),
System.currentTimeMillis(), bounceRequest.or(SingularityBounceRequest.defaultRequest()), actionId.get()));
System.currentTimeMillis(), validatedBounceRequest, actionId.get()));

return fillEntireRequest(requestWithState);
}
Expand Down