-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #3527] Modify the logic around the opportunity change tracking table to never delete records #3565
base: main
Are you sure you want to change the base?
[Issue #3527] Modify the logic around the opportunity change tracking table to never delete records #3565
Conversation
api/src/db/models/task_models.py
Outdated
class JobStatus(StrEnum): | ||
STARTED = "started" | ||
COMPLETED = "completed" | ||
FAILED = "failed" |
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.
Let's use our lookup tables for this, just requires setting up a boilerplatey table and a mapping in the lookup models file.
api/src/task/task.py
Outdated
if self.job is not None: | ||
self.job.job_status = JobStatus.FAILED | ||
self.db_session.commit() |
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.
Just to have slightly fewer places we put commits, what if we had this:
def update_job_status(self, job_status: JobStatus) -> None:
self.job.job_status = job_status
self.db_session.commit()
api/src/db/models/task_models.py
Outdated
job_id: Mapped[uuid.UUID] = mapped_column(UUID, primary_key=True, default=uuid.uuid4) | ||
job_type: Mapped[str] | ||
job_status: Mapped[str] |
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.
Just brainstorming.
Should job_type be an enum?
Should we store the metrics object as well as JSON?
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.
I was thinking about the metrics object and will add it for flexibility down the road (although I think NR is place to store and query these). When trying to make this an enum the migration would try creating these enum types that I wasn't sure was right. Will make this something like LkJobStatus
|
||
|
||
class JobTable(ApiSchemaTable, TimestampMixin): | ||
__tablename__ = "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.
Let's have a slightly longer name, what about task_log
or something like that?
… of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3527-modify-load-opp-logic-never-delete
Summary
Fixes #3527
Time to review: 30 mins
Changes proposed
Add a new table to track task progress
Rename queue table and add all opportunities to it. Add any new opportunities that come in (should basically always be 1:1)
For index job, load data based on
last_loaded_at
date rather than usinghas_update
field or deleting records as we go.Context for reviewers
Cascade deleting orphan relationship remains unchanged
JobTable is meant to be general purpose and can be used, possibly ended, by other tasks.
Additional information
See unit tests