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 started and finished times to Job #50

Closed
wants to merge 4 commits into from
Closed

Conversation

emilmelnikov
Copy link
Member

@emilmelnikov emilmelnikov commented Jan 22, 2020

  • Add "started_on" and "finished_on" fields to the Job model.
  • Add the "Running Time" column to the project job list view.

Other changes:

  • Remove the row number field from the project job list view:
    job ID should be sufficient for debugging.
  • Add full job ID tooltip.
  • Change submission time tooltip from short human-readable format
    to full date string in order to provide more information.

Closes #44.

@m-novikov
Copy link
Contributor

Great!
There is a problem though we use filter(...).update(...) invocation for state transitions. This won't call save method leaving finished and updated fields not populated.
We discussed it with @Tomaz-Vieira and the idea is to move state transitions to a custom model.Manager which will ensure we have our state machine in one place and everything uses the same pattern. filter(status=..., pk=...).update(status=new_status) and check number of matched rows.

(JobStatus.done, JobStatus.collected): {},
}

def update_status(self, *, old: JobStatus, new: JobStatus) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also contain and id argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should also contain and id argument.

Since querysets can be chained, filtering by ID can be done before updating where needed. This way, we can update status for any given queryset (e.g. for multiple jobs or by PK instead of external ID). Therefore, I suggest leaving update_status as a special case of generic update.

if "now" in transition_info:
extra_update_fields[transition_info["now"]] = datetime.datetime.utcnow()

return self.filter(status=old).update(status=new, **extra_update_fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

Check returned value is 1. Given that we have id based lookup.

Copy link
Contributor

@Tomaz-Vieira Tomaz-Vieira Jan 27, 2020

Choose a reason for hiding this comment

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

Maybe we should have each transition as its own specific method, since they look like they have transition-specific logic, like that if "now" in transition info. It might be easier to maintain and customize transitions that look like

class JobExecution:
    ...
    def transition_to_running(self):
        update_time = datetime.datetime.utcnow()
        num_records = self.filter(status=JobStatus.created).update(status=JobStatus.running, started_on=update_time)
        if num_records != 1:
            raise SomeException(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should have each transition as its own specific method, since they look like they have transition-specific logic, like that if "now" in transition info

I don't see any value for this right now because the transition-specific logic is minimal. Also, transitions are driven by a status value anyway.

* Add "started_on" and "finished_on" fields to the Job model.
* Add the "Running Time" column to the project job list view.

Other changes:

* Remove the row number field from the project job list view:
  job ID should be sufficient for debugging.
* Add full job ID tooltip.
* Change submission time tooltip from short human-readable format
  to full date string in order to provide more information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collect statistics on task duration
3 participants