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

add a default bounce expiration #866

Merged
merged 4 commits into from Feb 4, 2016
Merged

add a default bounce expiration #866

merged 4 commits into from Feb 4, 2016

Conversation

@ssalinas
Copy link
Member

ssalinas commented Jan 28, 2016

Don't allow a bounce that will go on forever. Defaults to 60 minutes and shows in the ui as:
screen shot 2016-01-28 at 9 14 55 am

@ssalinas ssalinas force-pushed the default_bounce_expire branch from b8c5543 to f6d95b9 Jan 28, 2016
actionId = bounceRequest.get().getActionId();
message = bounceRequest.get().getMessage();

if (bounceRequest.get().getDurationMillis().isPresent() && !actionId.isPresent()) {

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 28, 2016

Contributor

This logic was changed

This comment has been minimized.

Copy link
@ssalinas

ssalinas Jan 28, 2016

Author Member

Thanks, I'll check against staging to see what changed

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 28, 2016

Contributor

Ah, now that I look again - are we now saying it's impossible to have unlimited bounce?

If so, what will you do if someone enters a negative value for this configuration? Need to have validation on it or allow people to set it to 0/negative to indicate that they don't want to use this feature.

In which case, we'd only set actionId if there was a duration set.

This comment has been minimized.

Copy link
@ssalinas

ssalinas Jan 28, 2016

Author Member

Do you think we should still create an expiring object either way? One thing I realized in writing this is that if we don't create the expiring object, there isn't actually a way to cancel the bounce (without doing a janky pause/unpause)

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 28, 2016

Contributor

Good point, maybe just setup the config to validate that this must be non-zero, positive, then it's not an issue. I think you can do that with annotations. Someone could set it really high.

bounceRequest = new SingularityBounceRequest(
Optional.<Boolean>absent(),
Optional.<Boolean>absent(),
Optional.of(TimeUnit.MINUTES.toMillis(configuration.getDefaultBounceExpirationMinutes())),

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 28, 2016

Contributor

Instead of storing this in the object, why not check it later? Recreating the object like this is gross.

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 28, 2016

Contributor

Also, as a general rule, late-binding of configuration properties makes testing easier.

@ssalinas
Copy link
Member Author

ssalinas commented Jan 28, 2016

Ok, reworked this a bit. Not reconstructing the object anymore, just getting the durationMillis slightly differently for bounces in SingularityExpiringUserActionPoller so we can use the default and added a @Min for the config value. Tweaked the UI to show default value as well if there isn't a duration present for bounce

@tpetr tpetr modified the milestones: 0.4.9, 0.4.10 Feb 2, 2016
ssalinas added a commit that referenced this pull request Feb 4, 2016
add a default bounce expiration
@ssalinas ssalinas merged commit 283287f into master Feb 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ssalinas ssalinas deleted the default_bounce_expire branch Feb 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.