Guarantee durationMillis is present in getExpiringBounce response #1197

MattCCS · 2016-08-09T16:42:38Z

This PR guarantees that a bounce request's result always includes the durationMillis, whether a value was given or not. If no value was given, the durationMillis value is the appropriate default.

/cc @ssalinas @wolfd @tpetr

ssalinas · 2016-08-09T16:47:02Z

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

@@ -385,7 +390,31 @@ public SingularityDeleteResult deleteLbCleanupRequest(String requestId) {
  }

  public Optional<SingularityExpiringBounce> getExpiringBounce(String requestId) {
-    return getExpiringObject(SingularityExpiringBounce.class, requestId);
+    final Optional<SingularityExpiringBounce> optionalBounce = getExpiringObject(SingularityExpiringBounce.class, requestId);


more of a convention thing, we tend to name vars for optionals as maybe..., so in this case maybeExpiringBounce or something similar

tpetr · 2016-08-09T18:01:51Z

Are more than just expring bounces affected by the missing field issue? I feel like all expiring objects that have an absent durationMillis field could benefit.
What happens when you create an expiring bounce with no durationMillis set, and then you redeploy Singularity with a different default value? Will the right thing happen in every case?
Expanding on above, why not populate durationMillis right before it's written in ZK, instead of populating it after every read?

tpetr · 2016-08-09T18:08:57Z

SingularityBase/src/main/java/com/hubspot/singularity/expiring/SingularityExpiringBounce.java

@@ -20,6 +20,34 @@ public String getDeployId() {
    return deployId;
  }

+  public static Optional<SingularityExpiringBounce> withDefaultExpiringMillis(


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

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

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

…onse value

tpetr · 2016-08-09T20:21:34Z

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

@@ -201,8 +202,17 @@ public SingularityRequestParent bounce(@ApiParam("The request ID to bounce") @Pa

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

+    SingularityBounceRequest defaultBounceRequest = bounceRequest.or(SingularityBounceRequest.defaultRequest());
+    if (!defaultBounceRequest.getDurationMillis().isPresent()) {
+      final Long durationMillis = TimeUnit.MINUTES.toMillis(configuration.getDefaultBounceExpirationMinutes());


this is a nitpick, but you should utilize primitive types wherever possible (i.e. long instead of Long)

In this case I was following the convention established in the constructor of the SingularityBounceRequest class (@JsonProperty("durationMillis") Optional<Long> durationMillis). If I change it here, should I change it in the constructor as well?

When using it as a type in the Optional<> you have to use the generic object (Long) in that case. A primitive does not extend any Object so it cannot be the type argument there. But, when it isn't required to have the generic object (like in your case, just setting a variable and using the value) it is preferred to use the primitive. You can pass that primitive (long) as an arg for something that takes Long just fine. (i.e. Optional.of(yourVar))

^ 👍. The JVM's ability to hop between primitives (long) and "boxed" types (Long) is called auto-boxing. This is just one of those style things everyone learns the hard way 😃

tpetr · 2016-08-10T18:07:17Z

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

    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;


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

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

tpetr · 2016-08-24T21:15:37Z

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

@@ -80,7 +80,8 @@
  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) {


singularityConfiguration is not in use

tpetr · 2016-08-24T21:17:22Z

LGTM after last 2 comments are addressed

…inutes

MattCCS · 2016-08-24T22:15:43Z

Both comments have been addressed. Does this look good to go?

ssalinas · 2016-08-25T12:53:16Z

Looks good @MattCCS

tpetr · 2016-08-25T12:57:01Z

LGTM, good to merge once this has been re-merged / redeployed through the 3 environments

matush-v · 2016-08-25T14:55:16Z

SingularityBase/src/main/java/com/hubspot/singularity/api/SingularityBounceRequestBuilder.java

+        ", actionId='" + actionId + '\'' +
+        ", message=" + message + '\'' +
+        ", incremental=" + incremental + '\'' +
+        ", skipHealthchecks=" + skipHealthchecks + '\'' +


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

Guarantee durationMillis is present in getExpiringBounce response

77cb958

ssalinas reviewed Aug 9, 2016
View reviewed changes

MattCCS added 2 commits August 9, 2016 12:49

Rename optionalBounce -> maybeExpiringBounce

5afce17

Move ExpiringBounce defaultMillis logic out of RequestManager

298f71c

tpetr reviewed Aug 9, 2016
View reviewed changes

MattCCS added 2 commits August 9, 2016 16:11

Implement BounceRequestBuilder, update durationMillis of /bounce resp…

008eaa2

…onse value

Remove old ExpiringBounce durationMillis fix

a469d15

tpetr reviewed Aug 9, 2016
View reviewed changes

MattCCS added 3 commits August 10, 2016 14:00

Moved durationMillis fix logic into validator

0845b80

Change Long to long in durationMillis fix

e1ee388

Make durationMillis validator method name more appropriate

cb5cd7f

tpetr reviewed Aug 10, 2016
View reviewed changes

MattCCS added 2 commits August 10, 2016 14:11

Remove now-unnecessary SingularityConfiguration attribute

f7d5211

Rename validateBounceRequest -> checkBounceRequest

0d767d2

MattCCS added hs_staging labels Aug 10, 2016

ssalinas modified the milestone: 0.11.0 Aug 19, 2016

ssalinas added the hs_stable label Aug 22, 2016

tpetr reviewed Aug 24, 2016
View reviewed changes

MattCCS added 2 commits August 24, 2016 18:01

Remove redundant SingularityConfiguration param

df9182f

Remove SConfiguration property, replace with defaultBounceExpirationM…

2036fb6

…inutes

matush-v reviewed Aug 25, 2016
View reviewed changes

Fix missing apostrophes

591e238

MattCCS merged commit 5839bda into master Aug 25, 2016

tpetr removed hs_qa labels Aug 25, 2016

MattCCS deleted the bounce_millis_fix branch August 25, 2016 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guarantee durationMillis is present in getExpiringBounce response #1197

Guarantee durationMillis is present in getExpiringBounce response #1197

MattCCS commented Aug 9, 2016 •

edited

Loading

ssalinas Aug 9, 2016

MattCCS Aug 9, 2016

tpetr commented Aug 9, 2016

tpetr Aug 9, 2016

tpetr Aug 9, 2016 •

edited

Loading

tpetr Aug 9, 2016

MattCCS Aug 9, 2016

ssalinas Aug 9, 2016

tpetr Aug 9, 2016

tpetr Aug 10, 2016

MattCCS Aug 10, 2016

tpetr Aug 24, 2016

tpetr commented Aug 24, 2016

MattCCS commented Aug 24, 2016

ssalinas commented Aug 25, 2016

tpetr commented Aug 25, 2016

matush-v Aug 25, 2016

MattCCS Aug 25, 2016

Guarantee durationMillis is present in getExpiringBounce response #1197

Guarantee durationMillis is present in getExpiringBounce response #1197

Conversation

MattCCS commented Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpetr commented Aug 9, 2016

Choose a reason for hiding this comment

tpetr Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpetr commented Aug 24, 2016

MattCCS commented Aug 24, 2016

ssalinas commented Aug 25, 2016

tpetr commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattCCS commented Aug 9, 2016 •

edited

Loading

tpetr Aug 9, 2016 •

edited

Loading