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.

Sign up for GitHub

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

Guarantee durationMillis is present in getExpiringBounce response #1197

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
77cb958
Guarantee durationMillis is present in getExpiringBounce response
MattCCS Aug 9, 2016
5afce17
Rename optionalBounce -> maybeExpiringBounce
MattCCS Aug 9, 2016
298f71c
Move ExpiringBounce defaultMillis logic out of RequestManager
MattCCS Aug 9, 2016
008eaa2
Implement BounceRequestBuilder, update durationMillis of /bounce resp…
MattCCS Aug 9, 2016
a469d15
Remove old ExpiringBounce durationMillis fix
MattCCS Aug 9, 2016
0845b80
Moved durationMillis fix logic into validator
MattCCS Aug 10, 2016
e1ee388
Change Long to long in durationMillis fix
MattCCS Aug 10, 2016
cb5cd7f
Make durationMillis validator method name more appropriate
MattCCS Aug 10, 2016
f7d5211
Remove now-unnecessary SingularityConfiguration attribute
MattCCS Aug 10, 2016
0d767d2
Rename validateBounceRequest -> checkBounceRequest
MattCCS Aug 10, 2016
df9182f
Remove redundant SingularityConfiguration param
MattCCS Aug 24, 2016
2036fb6
Remove SConfiguration property, replace with defaultBounceExpirationM…
MattCCS Aug 24, 2016
591e238
Fix missing apostrophes
MattCCS Aug 25, 2016
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
28 changes: 28 additions & 0 deletions ...ularityBase/src/main/java/com/hubspot/singularity/expiring/SingularityExpiringBounce.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,34 @@ public String getDeployId() {
return deployId;
}

public static Optional<SingularityExpiringBounce> withDefaultExpiringMillis(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make a SingularityExpiringBounceBuilder object instead... that way you can do something like:

SingularityExpiringBounceBuilder.buildUpon(myExpiringBounceObject).setDurationMillis(Optional.of(7)).build()

Copy link
Contributor

@tpetr tpetr Aug 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

nevermind, i didn't realize this was so nested (edit: we talked offline and we're gonna do builders for both classes)

Optional<SingularityExpiringBounce> maybeExpiringBounce,
Long durationMillis) {
if (!maybeExpiringBounce.isPresent()) {
return maybeExpiringBounce;
}
final SingularityExpiringBounce bounce = maybeExpiringBounce.get();
if (bounce.getExpiringAPIRequestObject().getDurationMillis().isPresent()) {
return maybeExpiringBounce;
}
return Optional.of(
new SingularityExpiringBounce(
bounce.getRequestId(),
bounce.getDeployId(),
bounce.getUser(),
bounce.getStartMillis(),
new SingularityBounceRequest(
bounce.getExpiringAPIRequestObject().getIncremental(),
bounce.getExpiringAPIRequestObject().getSkipHealthchecks(),
Optional.of(durationMillis),
bounce.getExpiringAPIRequestObject().getActionId(),
bounce.getExpiringAPIRequestObject().getMessage()
),
bounce.getActionId()
)
);
}

@Override
public String toString() {
return "SingularityExpiringBounce [toString()=" + super.toString() + "]";
Expand Down
10 changes: 8 additions & 2 deletions SingularityService/src/main/java/com/hubspot/singularity/data/RequestManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;

import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.utils.ZKPaths;
Expand Down Expand Up @@ -53,6 +54,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 +82,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 Expand Up @@ -385,7 +389,9 @@ public <T extends SingularityExpiringParent<? extends SingularityExpiringRequest
}

public Optional<SingularityExpiringBounce> getExpiringBounce(String requestId) {
return getExpiringObject(SingularityExpiringBounce.class, requestId);
return SingularityExpiringBounce.withDefaultExpiringMillis(
getExpiringObject(SingularityExpiringBounce.class, requestId),
TimeUnit.MINUTES.toMillis(singularityConfiguration.getDefaultBounceExpirationMinutes()));
}

public Optional<SingularityExpiringPause> getExpiringPause(String requestId) {
Expand Down