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

Conversation

Projects
None yet
4 participants
@MattCCS
Contributor

MattCCS commented Aug 9, 2016

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

Show outdated Hide outdated ...tyService/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);

This comment has been minimized.

@ssalinas

ssalinas Aug 9, 2016

Member

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

@ssalinas

ssalinas Aug 9, 2016

Member

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

This comment has been minimized.

@MattCCS

MattCCS Aug 9, 2016

Contributor

Will fix

@MattCCS

MattCCS Aug 9, 2016

Contributor

Will fix

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Aug 9, 2016

Member
  • 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?
Member

tpetr commented Aug 9, 2016

  • 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?
Show outdated Hide outdated ...ain/java/com/hubspot/singularity/expiring/SingularityExpiringBounce.java
@@ -20,6 +20,34 @@ public String getDeployId() {
return deployId;
}
public static Optional<SingularityExpiringBounce> withDefaultExpiringMillis(

This comment has been minimized.

@tpetr

tpetr Aug 9, 2016

Member

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

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

tpetr Aug 9, 2016

Member

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

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

This comment has been minimized.

@tpetr

tpetr Aug 9, 2016

Member

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

@tpetr

tpetr Aug 9, 2016

Member

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

Show outdated Hide outdated ...ice/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 comment has been minimized.

@tpetr

tpetr Aug 9, 2016

Member

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

@tpetr

tpetr Aug 9, 2016

Member

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

This comment has been minimized.

@MattCCS

MattCCS Aug 9, 2016

Contributor

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?

@MattCCS

MattCCS Aug 9, 2016

Contributor

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?

This comment has been minimized.

@ssalinas

ssalinas Aug 9, 2016

Member

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))

@ssalinas

ssalinas Aug 9, 2016

Member

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))

This comment has been minimized.

@tpetr

tpetr Aug 9, 2016

Member

^ 👍. 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

tpetr Aug 9, 2016

Member

^ 👍. 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 😃

Show outdated Hide outdated ...ice/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());

This comment has been minimized.

@ssalinas

ssalinas Aug 10, 2016

Member

last nitpick on this, since this can be the user-provided data or the default, default doesn't make sense as a variable name. Maybe something like effectiveBounceRequest (or something similar??)

@ssalinas

ssalinas Aug 10, 2016

Member

last nitpick on this, since this can be the user-provided data or the default, default doesn't make sense as a variable name. Maybe something like effectiveBounceRequest (or something similar??)

Show outdated Hide outdated ...tyService/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;

This comment has been minimized.

@tpetr

tpetr Aug 10, 2016

Member

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

@tpetr

tpetr Aug 10, 2016

Member

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

This comment has been minimized.

@MattCCS

MattCCS Aug 10, 2016

Contributor

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

@MattCCS

MattCCS Aug 10, 2016

Contributor

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

Show outdated Hide outdated ...ice/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
@@ -436,4 +440,15 @@ private boolean isValidInteger(String strValue) {
return false;
}
}
public SingularityBounceRequest validateBounceRequest(SingularityBounceRequest defaultBounceRequest) {

This comment has been minimized.

@tpetr

tpetr Aug 10, 2016

Member

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

@tpetr

tpetr Aug 10, 2016

Member

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

This comment has been minimized.

@MattCCS

MattCCS Aug 10, 2016

Contributor

Will fix!

@MattCCS

MattCCS Aug 10, 2016

Contributor

Will fix!

@ssalinas ssalinas modified the milestone: 0.11.0 Aug 19, 2016

@ssalinas ssalinas added the hs_stable label Aug 22, 2016

Show outdated Hide outdated ...tyService/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) {

This comment has been minimized.

@tpetr

tpetr Aug 24, 2016

Member

singularityConfiguration is not in use

@tpetr

tpetr Aug 24, 2016

Member

singularityConfiguration is not in use

Show outdated Hide outdated ...ice/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
@@ -66,6 +68,8 @@
@Inject
public SingularityValidator(SingularityConfiguration configuration, DeployHistoryHelper deployHistoryHelper, RequestManager requestManager) {
this.configuration = configuration;

This comment has been minimized.

@tpetr

tpetr Aug 24, 2016

Member

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).

@tpetr

tpetr Aug 24, 2016

Member

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).

This comment has been minimized.

@MattCCS

MattCCS Aug 24, 2016

Contributor

And define this variable in SingularityConfiguration?

@MattCCS

MattCCS Aug 24, 2016

Contributor

And define this variable in SingularityConfiguration?

This comment has been minimized.

@tpetr

tpetr Aug 24, 2016

Member

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

@tpetr

tpetr Aug 24, 2016

Member

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

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Aug 24, 2016

Member

LGTM after last 2 comments are addressed

Member

tpetr commented Aug 24, 2016

LGTM after last 2 comments are addressed

@MattCCS

This comment has been minimized.

Show comment
Hide comment
@MattCCS

MattCCS Aug 24, 2016

Contributor

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

Contributor

MattCCS commented Aug 24, 2016

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

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Aug 25, 2016

Member

Looks good @MattCCS

Member

ssalinas commented Aug 25, 2016

Looks good @MattCCS

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Aug 25, 2016

Member

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

Member

tpetr commented Aug 25, 2016

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

Show outdated Hide outdated ...in/java/com/hubspot/singularity/api/SingularityBounceRequestBuilder.java
", actionId='" + actionId + '\'' +
", message=" + message + '\'' +
", incremental=" + incremental + '\'' +
", skipHealthchecks=" + skipHealthchecks + '\'' +

This comment has been minimized.

@darcatron

darcatron Aug 25, 2016

Contributor

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

@darcatron

darcatron Aug 25, 2016

Contributor

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

This comment has been minimized.

@MattCCS

MattCCS Aug 25, 2016

Contributor

Will fix

@MattCCS

MattCCS Aug 25, 2016

Contributor

Will fix

@MattCCS MattCCS merged commit 5839bda into master Aug 25, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@MattCCS MattCCS deleted the bounce_millis_fix branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment