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

Jump to bottom

GAWB-662 new workflow queue status endpoint #420

Merged
merged 1 commit into from
May 13, 2016
Merged

GAWB-662 new workflow queue status endpoint #420

merged 1 commit into from
May 13, 2016

Conversation

jmthibault79
Copy link
Contributor

@jmthibault79 jmthibault79 commented May 12, 2016 •
edited
Loading

Rawls portion of GAWB-662. Needs Orch + UI as well.

  • Submitter: Rebase to develop. DO NOT SQUASH
  • Submitter: Make sure Swagger is updated if API changes
  • Submitter: Make sure documentation for code is complete
  • Submitter: Make sure liquibase is updated if appropriate
  • Submitter: Review code comments; remove done TODOs, create stories for remaining TODOs
  • Submitter: Include the JIRA issue number in the PR description
  • Submitter: Add description or comments on the PR explaining the hows/whys (if not obvious)
  • Tell that the PR exists if he wants to look at it (apply requires_doge label)
  • Anoint a lead reviewer (LR). Assign PR to LR
  • LR: Initial review by LR and others.
  • Comment / review / update cycle:
    • Rest of team may comments on PR at will
    • LR assigns to submitter for feedback fixes
    • Submitter updates documentation as needed
    • Submitter rebases to develop again if necessary
    • Submitter makes further commits. DO NOT SQUASH. Reassign to LR for further feedback
  • sign off
  • LR sign off
  • Assign to submitter to finalize
  • Submitter: Squash commits, rebase if necessary
  • Submitter: Verify all tests go green, including CI tests
  • Submitter: Merge to develop
  • Submitter: Delete branch after merge
  • Submitter: Check configuration files in Jenkins in case they need changes
  • Submitter: Test this change works on dev environment after deployment. YOU own getting it fixed if dev isn't working for ANY reason!
  • Submitter: Verify swagger UI on dev environment still works after deployment
  • Submitter: Inform other teams of any API changes via hipchat and/or email
  • Submitter: Mark JIRA issue as resolved once this checklist is completed

jmthibault79
jmthibault79 reviewed May 12, 2016
View reviewed changes
src/main/scala/org/broadinstitute/dsde/rawls/model/ExecutionModel.scala
@@ -304,6 +305,8 @@ object WorkflowStatuses {

def withName(name: String): WorkflowStatus = {
name match {
case "Queued" => Queued
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@helgridly this change requested by @abaumann

Copy link
Contributor

Choose a reason for hiding this comment

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

I put this request into the queue doc - I want to settle on what we are going to call these states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you link the doc? I've lost track of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.google.com/document/d/19IkGcrrRhKyjTr5WtV0V6vaV6ihonHrlswN9kPrt0hc/edit

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 78.166% when pulling d18d885ac4f3b041b518250b7d284bf74c4aefd9 on jt_GAWB-662 into 1bac55e on develop.

dmohs
dmohs reviewed May 12, 2016
View reviewed changes
src/main/resources/swagger/rawls.yaml
@@ -1088,6 +1088,28 @@ paths:
- openid
- email
- profile
'/api/workflow_queue_status':
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 use a consistent style for URLs. I filed GAWB-673 for this, but I wouldn't complain if you changed this to workflow-queue-status in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change it to that. Curious what others think too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rawls seems to consistently use camelCase, from what I can tell from Swagger

@helgridly
Copy link
Contributor

If this is for babyQ, I've created branch FEAT_668_bbq and I propose we merge babyQ PRs into there.

@dmohs
Copy link
Contributor

dmohs commented May 12, 2016

👍

@dmohs dmohs assigned jmthibault79 and unassigned dmohs May 12, 2016
@jmthibault79
Copy link
Contributor Author

It is for PorQ/BBQ but I think it's safe to go to develop as well, as it doesn't change any functionality.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 78.166% when pulling bf28e0858c93ac857c66d4731c2ea3bdfe2426b3 on jt_GAWB-662 into 1bac55e on develop.

dvoet
dvoet reviewed May 13, 2016
View reviewed changes
src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkflowComponent.scala
findQueuedAndRunningWorkflows.result.map { recs =>
val grouped = recs.groupBy {
// lump all of the workflows currently being handled by Cromwell into "Active"
case runningRecord if WorkflowStatuses.runningStatuses.map(_.toString).toSet contains runningRecord.status => "Active"
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we doing this lumping here? I think this should be a straight group by in the db and let the caller (ui?) figure out how they want to lump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's convenient to do so in Rawls because we have statuses partitioned by Queued / Running / Terminal in WorkflowStatuses.

I can see how this would be more extensible if we simply included every status and let callers/UI handle partitioning. In that case I'd modify this further to include the Terminal statuses as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

terminal statuses are not particularly informative of anything, will be large and may make the query slower. The only one I care about is the last one though. If the query is not slower then I don't really care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lumping removed

@jmthibault79 jmthibault79 assigned dvoet and unassigned jmthibault79 May 13, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 78.204% when pulling 06eb6fa0a06eba6bf2b05722f275b492adbbef61 on jt_GAWB-662 into 1bac55e on develop.

@dvoet
Copy link
Contributor

dvoet commented May 13, 2016

+1

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 8dc2890 on jt_GAWB-662 into * on develop*.

@jmthibault79 jmthibault79 merged commit 823628c into develop May 13, 2016
@jmthibault79 jmthibault79 deleted the jt_GAWB-662 branch May 13, 2016 20:40
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 78.062% when pulling 8dc2890 on jt_GAWB-662 into cd25e24 on develop.

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

@dvoet dvoet

Labels
requires_doge
Projects
None yet
Milestone
No milestone
6 participants
@jmthibault79 @coveralls @helgridly @dmohs @dvoet @abaumann