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 flag to run scheduled task on deploy #1515

Merged
merged 8 commits into from Jun 26, 2017

Conversation

Projects
None yet
2 participants
@PtrTeixeira
Contributor

PtrTeixeira commented Apr 25, 2017

Adds a field in the deploy, runImmediately, which takes a
SingularityRunNowRequest. If that field is present, then the scheduled
task (or on-demand task) will be run immediately after deploying.

/cc @ssalinas

Add flag to run scheduled task on deploy
Adds a field in the deploy, `runImmediately`, which takes a
`SingularityRunNowRequest`. If that field is present, then the scheduled
task (or on-demand task) will be run immediately after deploying.
@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Apr 25, 2017

Member

A few things on this one:

  • When doing a regular run now, we do a number of validations on the run now request. We should imitate this to some extent in the deploy case as well, probably split between validations when we receive the request and when we try to enqueue it in the pending queue
    • If the scheduled job is already running we shouldn't be enqueueing another pending request. I don't think this needs to fail the deploy though. Maybe we can name it something like attemptToRunImmediately or find some way of making it clear it is a 'best effort'
    • We should hydrate the run now request with an id if one is not set
    • A number of the badRequest checks can be moved to the validator and we can reuse that code to check the run now request on the deploy object if it is present (like the length of runId one)
Member

ssalinas commented Apr 25, 2017

A few things on this one:

  • When doing a regular run now, we do a number of validations on the run now request. We should imitate this to some extent in the deploy case as well, probably split between validations when we receive the request and when we try to enqueue it in the pending queue
    • If the scheduled job is already running we shouldn't be enqueueing another pending request. I don't think this needs to fail the deploy though. Maybe we can name it something like attemptToRunImmediately or find some way of making it clear it is a 'best effort'
    • We should hydrate the run now request with an id if one is not set
    • A number of the badRequest checks can be moved to the validator and we can reuse that code to check the run now request on the deploy object if it is present (like the length of runId one)

@ssalinas ssalinas modified the milestone: 0.16.0 May 1, 2017

Validate run immediately request
Provide more validation for the run immediately request. Also
centralizes the validation of both the run-now request and the
run-immediately-on-deploy request in the SingularityValidator class.
Also also also adds a few more tests in Singularity Validator.
@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas May 1, 2017

Member

+1 for consolidating in the validator. Let's fix the merge conflicts and give this a try in staging

Member

ssalinas commented May 1, 2017

+1 for consolidating in the validator. Let's fix the merge conflicts and give this a try in staging

PtrTeixeira added some commits May 1, 2017

Catch missing cases
Catch missing cases for running once-off tasks. In particular, I had
assummed `request.isOneOff` included both on-demand and run-once tasks,
which it doesn't.
Show outdated Hide outdated ...ain/java/com/hubspot/singularity/scheduler/SingularityDeployChecker.java
SingularityRunNowRequest runNowRequest = maybeRunNowRequest.get();
pendingType = PendingType.ONEOFF;
runId = runNowRequest.getRunId().or(Optional.of(UUID.randomUUID().toString()));
message = runNowRequest.getMessage()

This comment has been minimized.

@ssalinas

ssalinas May 22, 2017

Member

might be going a bit line-break happy here/above/below in this method ;) can get a bit hard to read

@ssalinas

ssalinas May 22, 2017

Member

might be going a bit line-break happy here/above/below in this method ;) can get a bit hard to read

This comment has been minimized.

@PtrTeixeira

PtrTeixeira May 22, 2017

Contributor

Just couldn't wait to get started with immutables c:

@PtrTeixeira

PtrTeixeira May 22, 2017

Contributor

Just couldn't wait to get started with immutables c:

Show outdated Hide outdated ...ain/java/com/hubspot/singularity/scheduler/SingularityDeployChecker.java
pendingDeploy,
deployResult,
deploy.get().getRunImmediately());
if (maybePendingRequest.isPresent()) {

This comment has been minimized.

@ssalinas

ssalinas May 22, 2017

Member

for this and the one below. We should always make sure some pending request gets enqueued for a !deployable && !oneOff case. If it doesn't get enqueued it's possible we could miss scheduling the next run of a cron or something like that.

@ssalinas

ssalinas May 22, 2017

Member

for this and the one below. We should always make sure some pending request gets enqueued for a !deployable && !oneOff case. If it doesn't get enqueued it's possible we could miss scheduling the next run of a cron or something like that.

Show outdated Hide outdated ...ice/src/main/java/com/hubspot/singularity/data/SingularityValidator.java
throw badRequest("Can not request an immediate run of a non-scheduled / always running request (%s)", request);
}
boolean canRunImmediately =

This comment has been minimized.

@ssalinas

ssalinas May 22, 2017

Member

do we want to validate this at the time the deploy is enqueued? We re-check similar conditions at the time the pending request is created when the deploy finishes, and the data could have changed by then. Checking here also causes a few extra zk calls (activeTaskIds/pendingTaskIds) that could possibly be hit on the non-leader instance

@ssalinas

ssalinas May 22, 2017

Member

do we want to validate this at the time the deploy is enqueued? We re-check similar conditions at the time the pending request is created when the deploy finishes, and the data could have changed by then. Checking here also causes a few extra zk calls (activeTaskIds/pendingTaskIds) that could possibly be hit on the non-leader instance

@PtrTeixeira PtrTeixeira added hs_qa and removed hs_qa labels May 22, 2017

PtrTeixeira added some commits May 23, 2017

Repond to PR comments
Respond to PR comments. In particular
- Style change: reduce the number of line breaks in method calls
- Don't validate task count on enqueue; defer it to the deploy is going to be run
- Guaranteed that _something_ will be deployed, even when a task couldn't
  be run.

@ssalinas ssalinas merged commit e7cec0c into master Jun 26, 2017

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 run-after-deploy branch Jun 26, 2017

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