Skip to content

Feat: Logic for finding the next request to be processed and tests #2166

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jamesbarford
Copy link
Contributor

Tees up the next request in the benchmark_request table (does not create jobs in what will be the job_queue table.

  • Tests for database
  • Tests for the sorting
    • On sorting; I did implement the sorting logic in SQL - it's probably more performant however it's about 60ish lines and would be fairly difficult to maintain. As we shouldn't be pull back vast amounts of data, and we only do this when something is not in_progress I think we should be ok? Though I can bring back the SQL version if needed.
  • I couldn't see an obvious path for stubbing out some of the fields on SiteCtxt thus split the sorting logic to; get_next_benchmark_request(...)
  • We want to rename waiting_for_parent, I'm a bit lost as to what the name should be so am very open to suggestions. waiting along with some code comments about what waiting means for the different types of commits was the best I could think of however I held off

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Left a few comments, but it would be best to discuss in sync I think.

@Kobzol
Copy link
Member

Kobzol commented Jun 30, 2025

I think we can discuss the sorting logic in sync, now that we can take a look at existing code.

@Jamesbarford Jamesbarford force-pushed the feat/establish-commit-hierarchy branch from c0d4708 to fcb6f17 Compare July 2, 2025 14:30
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Left a few comments, but it looks pretty closely to how I envisioned it. I was a bit confused by the comments about "30 days" in tests though.

@Jamesbarford Jamesbarford force-pushed the feat/establish-commit-hierarchy branch 2 times, most recently from 7eb4fe9 to 5d8da27 Compare July 3, 2025 08:51
@Kobzol
Copy link
Member

Kobzol commented Jul 4, 2025

Thank you! Please squash the commits and we can merge this.

@Jamesbarford Jamesbarford force-pushed the feat/establish-commit-hierarchy branch 2 times, most recently from b68cdfc to fbd79ff Compare July 6, 2025 04:51
@Jamesbarford Jamesbarford force-pushed the feat/establish-commit-hierarchy branch from fbd79ff to f64dbe8 Compare July 6, 2025 04:56
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.

2 participants