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

[Issue #3527] Modify the logic around the opportunity change tracking table to never delete records #3565

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mikehgrantsgov
Copy link
Collaborator

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 using has_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

Comment on lines 11 to 14
class JobStatus(StrEnum):
STARTED = "started"
COMPLETED = "completed"
FAILED = "failed"
Copy link
Collaborator

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.

Comment on lines 66 to 68
if self.job is not None:
self.job.job_status = JobStatus.FAILED
self.db_session.commit()
Copy link
Collaborator

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()

Comment on lines 20 to 22
job_id: Mapped[uuid.UUID] = mapped_column(UUID, primary_key=True, default=uuid.uuid4)
job_type: Mapped[str]
job_status: Mapped[str]
Copy link
Collaborator

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?

Copy link
Collaborator Author

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"
Copy link
Collaborator

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?

@mikehgrantsgov mikehgrantsgov marked this pull request as ready for review January 17, 2025 22:04
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.

Modify the logic around the opportunity change tracking table to never delete records
3 participants