Skip to content
This repository was archived by the owner on Nov 4, 2021. It is now read-only.

Big refactor #2 + Jobs Jobs Jobs - #351

Merged
mariusandra merged 15 commits into
masterfrom
retries-to-jobs
May 4, 2021
Merged

Big refactor #2 + Jobs Jobs Jobs#351
mariusandra merged 15 commits into
masterfrom
retries-to-jobs

Conversation

@mariusandra

@mariusandra mariusandra commented Apr 30, 2021

Copy link
Copy Markdown
Collaborator

Changes

  • Should be merged after Big refactor #350 is merged, which should be merged after Retry queue #325 is merged
  • Rename "retry queue" to "job queue"
  • Refactor worker tasks (get rid of the huge if, replace it with a smaller object... one step at a time)
  • Implements the following syntax for plugin jobs:
export const jobs = {
    retryProcessEvent: (event, meta) => {
        console.log('retrying event!', event.event)
    }
}
export async function processEvent (event, meta) {
    if (event.properties?.hi === 'ha') {
        console.log('processEvent')
        // queues to run in 30 seconds
        meta.jobs.retryProcessEvent(event).runIn(30, 'seconds')
        // queues to run async but now
        meta.jobs.retryProcessEvent(event).runNow()
    }
    return event
}

I'm also very happy to discuss changes to the API. This is the PR to do that.

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@mariusandra mariusandra mentioned this pull request Apr 30, 2021
2 tasks
@mariusandra mariusandra added the bump minor Bump minor version when this PR gets merged label Apr 30, 2021
Comment thread src/worker/vm/extensions/jobs.ts Outdated
return new Proxy(
{},
{
get: (target, key) => async (payload: Record<string, any>) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the spirit of PostHog/plugin-scaffold#16 (review), could runIn not take target and key directly?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hm... we don't really use target at all, so I assume you mean payload? I mean we can have:

job.runIn(3, 'minutes', 'taskName', { task: 'payload' })

... but doesn't really spark joy.

job.runIn(3, 'minutes').taskName({ task: 'payload' })

Not sure, it's all very subjective :). Feel free to make some plugin and play around with alternative APIs for this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, we can also flip it around:

jobs.taskName({ task: 'payload' }).runIn(3, 'minutes')
jobs.taskName({ task: 'payload' }).runNow()

@mariusandra mariusandra removed the bump minor Bump minor version when this PR gets merged label May 3, 2021
Base automatically changed from big-refactor to master May 3, 2021 11:14
@mariusandra

mariusandra commented May 3, 2021

Copy link
Copy Markdown
Collaborator Author

All right, the last changes in this PR flip the functions around and now this is how it works:

export const jobs = {
    retryProcessEvent: (event, meta) => {
        console.log('retrying event!', event.event)
    }
}

export async function processEvent (event, { jobs }) {
    if (event.properties?.hi === 'ha') {
        console.log('processEvent')

        // await to be sure it got enqueued, otherwise skip it and YOLO

        await jobs.retryProcessEvent(event).runIn(30, 'seconds')
        await jobs.retryProcessEvent(event).runNow()
        await jobs.retryProcessEvent(event).runAt(new Date())
    }
    return event
}

@yakkomajuri @Twixes would this syntax work for you? In case tests pass (this PR includes some flake fixes and other cleanup), I might merge this anyway and we can always tweak it later this week.

@yakkomajuri yakkomajuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quite like the new syntax - also happy to merge and iterate if necessary

@mariusandra

Copy link
Copy Markdown
Collaborator Author

Just tested this with Amazon Aurora Serverless (thanks @fuziontech) and it actually works with it! 🎉
We should run some experimental load tests to see what it can handle.

In any case, I'd still like to improve the layering and fallback system for these job queues. So plenty of refactoring still to come.

@mariusandra mariusandra added the bump minor Bump minor version when this PR gets merged label May 3, 2021
@mariusandra
mariusandra requested a review from Twixes May 4, 2021 06:07
Comment thread src/worker/tasks.ts
Comment on lines +12 to +14
hello: (server, args) => {
return `hello ${args}!`
},

@Twixes Twixes May 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be in here, as just hello? Seems a bit cryptic/odd

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That can be most likely deleted. I just copied this from the previous place. Next time, as there are more refactors to come.

@Twixes

Twixes commented May 4, 2021

Copy link
Copy Markdown
Contributor

That flipped API looks pretty great 👌 @mariusandra

@mariusandra
mariusandra merged commit d869516 into master May 4, 2021
@mariusandra
mariusandra deleted the retries-to-jobs branch May 4, 2021 14:05
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…PostHog/plugin-server#351)

* Rename "retry" to "jobs", improve API for scheduling jobs.

* refactor jobs test

* more debug for redlock

* turn the job code around

* add `.runAt` method

* add tests for the three methods: runNow, runAt and runIn

* use variable delay

* fix unintentionally broken test

* better ways to throw errors

* log a bit more on error in tests

* use graphile url for consumer, refactor queues slightly

* move connect to server

* silence warning for now

* fix default config in GH tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bump minor Bump minor version when this PR gets merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants