-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Generated by Django 2.2.9 on 2020-01-22 08:27 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('batch', '0006_auto_20200117_1459'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='job', | ||
name='finished_on', | ||
field=models.DateTimeField(blank=True, editable=False, null=True), | ||
), | ||
migrations.AddField( | ||
model_name='job', | ||
name='started_on', | ||
field=models.DateTimeField(blank=True, editable=False, null=True), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import uuid | ||
import datetime | ||
import enum | ||
import uuid | ||
|
||
from django.contrib.auth import get_user_model | ||
from django.core.exceptions import ValidationError | ||
|
@@ -82,6 +83,34 @@ def choices(cls): | |
return [(key.value, key.name) for key in cls] | ||
|
||
|
||
class JobManager(models.Manager): | ||
_STATUS_TRANSITIONS = { | ||
(JobStatus.created, JobStatus.running): {"now": "started_on"}, | ||
(JobStatus.running, JobStatus.done): {"now": "finished_on"}, | ||
(JobStatus.running, JobStatus.failed): {"now": "finished_on"}, | ||
(JobStatus.done, JobStatus.collected): {}, | ||
} | ||
|
||
def update_status(self, *, old: JobStatus, new: JobStatus) -> int: | ||
"""Update the job status of records with the old status to the new status. | ||
|
||
Returns: | ||
Number of changed rows. | ||
|
||
Raises: | ||
ValueError: Status transition is invalid. | ||
""" | ||
transition_info = self._STATUS_TRANSITIONS.get((old, new)) | ||
if transition_info is None: | ||
raise ValueError(f"invalid job status transition from {old.value} to {new.value}") | ||
|
||
extra_update_fields = {} | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check returned value is 1. Given that we have id based lookup. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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(...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
|
||
class Job(models.Model): | ||
""" | ||
Represents remote job | ||
|
@@ -93,12 +122,16 @@ class Job(models.Model): | |
owner = models.ForeignKey(User, on_delete=models.PROTECT, null=True, blank=True, editable=False) | ||
created_on = models.DateTimeField(auto_now_add=True, editable=False) | ||
updated_on = models.DateTimeField(auto_now=True, editable=False) | ||
started_on = models.DateTimeField(editable=False, null=True, blank=True) | ||
finished_on = models.DateTimeField(editable=False, null=True, blank=True) | ||
|
||
# TODO: Should be many to many relation with roles | ||
raw_data = models.ForeignKey( | ||
"datasets.Dataset", editable=False, null=True, blank=False, on_delete=models.PROTECT, related_name="+" | ||
) | ||
project = models.ForeignKey(Project, editable=False, null=True, blank=False, on_delete=models.PROTECT) | ||
|
||
objects = JobManager() | ||
|
||
class Meta: | ||
verbose_name = "Job" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 genericupdate
.