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

feat(orchestration): introducing the scheduler #2132

Merged
merged 3 commits into from May 13, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented May 10, 2024

This is the first stage of the orchestration v2, implementing the scheduler.
As agreed the first version of the scheduler doesn't support recurring tasks, only tasks that are scheduled immediately.

scheduler package is not used yet but it is big enough to warrant a review. There is still a few TODOs, which I will address in subsequent PRs

Issue ticket number and link

https://linear.app/nango/document/[brief]-orchestration-v2-4c5d1f1e104b#94b7a156-de22-4ce2-8a77-9cfcfe1fcece

Checklist before requesting a review (skip if just adding/editing APIs & templates)

  • I added tests, otherwise the reason is:
  • I added observability, otherwise the reason is:
  • I added analytics, otherwise the reason is:

@TBonnin TBonnin force-pushed the tbonnin/orchestration-v2.0 branch 7 times, most recently from 59b50d5 to ff4b239 Compare May 10, 2024 14:44
@TBonnin TBonnin changed the title orchestration v2 first stage: scheduler feat(orchestration): introducing the scheduler May 10, 2024
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Massive 👏🏻
Did not had time to test everything, but here is my first batch of comments

package.json Show resolved Hide resolved
packages/scheduler/package.json Outdated Show resolved Hide resolved
packages/scheduler/lib/vitest.d.ts Outdated Show resolved Hide resolved
packages/scheduler/lib/monitor.worker.ts Outdated Show resolved Hide resolved
packages/scheduler/lib/monitor.worker.ts Show resolved Hide resolved
packages/scheduler/lib/scheduler.ts Show resolved Hide resolved
packages/scheduler/lib/scheduler.ts Show resolved Hide resolved
packages/scheduler/lib/scheduler.integration.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Finished reading, I'll let you reply and adjust what you feel relevant. Ping me when you want a second review ☺️

packages/scheduler/lib/models/tasks.integration.test.ts Outdated Show resolved Hide resolved
packages/scheduler/lib/models/tasks.ts Outdated Show resolved Hide resolved
packages/scheduler/lib/models/tasks.ts Outdated Show resolved Hide resolved
packages/scheduler/lib/models/tasks.ts Outdated Show resolved Hide resolved
@TBonnin TBonnin force-pushed the tbonnin/orchestration-v2.0 branch from ff4b239 to ad1e099 Compare May 13, 2024 14:37
@TBonnin
Copy link
Collaborator Author

TBonnin commented May 13, 2024

I addressed all your comments @bodinsamuel

Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Let's go 🚀

@TBonnin TBonnin force-pushed the tbonnin/orchestration-v2.0 branch from ad1e099 to 1fe2f9d Compare May 13, 2024 17:56
@TBonnin TBonnin enabled auto-merge (squash) May 13, 2024 17:56
@TBonnin TBonnin merged commit 06c45c2 into master May 13, 2024
19 checks passed
@TBonnin TBonnin deleted the tbonnin/orchestration-v2.0 branch May 13, 2024 18:01
})
.forUpdate()
.skipLocked()
.debug(true)
Copy link
Member

Choose a reason for hiding this comment

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

Intentional I presume to leave this in while under development @TBonnin ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no actually it is some dev leftover. I will remove it

@khaliqgant
Copy link
Member

Looks good! Tests 💯

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.

None yet

3 participants