Skip to content

add: Round service #48

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 26 commits into
base: main
Choose a base branch
from
Open

add: Round service #48

wants to merge 26 commits into from

Conversation

pyropy
Copy link
Contributor

@pyropy pyropy commented Apr 14, 2025

This pull request introduces the new round service and the the new checker_rounds database table. The changes include configuration updates, the implementation of the round, database schema modifications, and new tests to ensure the functionality of the round service.

Round service will create a new round in the given interval. Upon creation of the new round task sampling is triggered and old rounds (if there are any) are set to inactive whilst the new round is set to active.

Round service should run in the background and generate the new rounds until the process is stopped.

Change log:

  • Implemented the RoundService class in lib/round-service.js to manage the lifecycle of rounds, including starting, stopping, and transitioning between rounds.
  • Added the TaskingService class in lib/tasking-service.js to handle task generation for each round, although the task generation logic is not yet implemented.
  • Added roundServiceConfig and taskingServiceConfig to poolConfig in lib/config.js to configure round duration, maximum tasks per node, and interval for checking round status.
  • Created a new table checker_rounds in migrations/003.do.checker-rounds.sql to store round information, including start and end times, active status, and maximum tasks per node.
  • Added tests for the RoundService in test/round-service.test.js to verify the creation and resumption of rounds, stopping the service, and transitioning between rounds.

Closes #35
Related to:

@pyropy pyropy self-assigned this Apr 14, 2025
@pyropy pyropy marked this pull request as ready for review April 14, 2025 13:06
@pyropy pyropy changed the title WIP: add round service add: Round service Apr 14, 2025
@pyropy pyropy requested review from bajtos and Copilot April 14, 2025 13:30
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • migrations/003.do.checker-rounds.sql: Language not supported

@pyropy pyropy requested a review from Copilot April 14, 2025 13:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • migrations/003.do.checker-rounds.sql: Language not supported

@pyropy pyropy mentioned this pull request Apr 14, 2025
try {
const { rows } = await this.#db.query(`
SELECT * FROM checker_rounds
WHERE active = true
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the active field? Can we simplify this to querying what is the last round where end_time > NOW()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Motivation behind the active field was to switch on round to active after the task creation. Unlike the implementation in the spark-api where tasks are created during the round creation on the database level, here we call TaskingService to create tasks for the round.

Thing that I'm questioning is should we or should we not leave the last round as active in case the TaskingService fails to sample / insert tasks for the new round?

Copy link
Member

Choose a reason for hiding this comment

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

Got it, you can only consider the round active once tasks have been sampled, but you can also only sample tasks once the round exists, right?

Can we then rename active to has_had_tasks_defined? This way the purpose to me is more clear. Another way is to JOIN on the tasks for that round, and simply consider any round as inactive if it has no matching tasks. This assumes we don't want to have empty rounds.

@pyropy pyropy requested review from juliangruber and Copilot April 15, 2025 13:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • migrations/003.do.checker-rounds.sql: Language not supported
Comments suppressed due to low confidence (1)

lib/round-service.js:144

  • The log statement references 'round.startTime', but the returned property from the SQL query is 'start_time'. Update the log to use 'round.start_time' for consistency.
console.log(`Created new round #${round.id} starting at ${round.startTime}`)

@@ -0,0 +1,184 @@
/** @typedef {{id: number; startTime: string; endTime: string; isExpired: Boolean; }} Round */
/** @typedef {{ roundDurationMs: number; checkRoundExpirationIntervalMs: number }} RoundConfig */
Copy link
Member

Choose a reason for hiding this comment

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

Can this be inferred from lib/config.js? Something like type RoundConfig = typeof config.roundServiceConfig

/**
* @type {NodeJS.Timeout | null}
*/
checkRoundExpirationInterval = null
Copy link
Member

Choose a reason for hiding this comment

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

why is this key not private as well?

Comment on lines +66 to +70
if (currentRound && !currentRound.isExpired) {
return
}

await this.#startNewRound()
Copy link
Member

Choose a reason for hiding this comment

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

This code is repeated in #initializeRound() can we refactor into a function #maybeStartNewRound()?

try {
const { rows } = await this.#db.query(`
SELECT * FROM checker_rounds
WHERE active = true
Copy link
Member

Choose a reason for hiding this comment

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

Got it, you can only consider the round active once tasks have been sampled, but you can also only sample tasks once the round exists, right?

Can we then rename active to has_had_tasks_defined? This way the purpose to me is more clear. Another way is to JOIN on the tasks for that round, and simply consider any round as inactive if it has no matching tasks. This assumes we don't want to have empty rounds.

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.

Create round system
2 participants