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

Background jobs rewrite: no Meteor dependencies! #5580

Merged
merged 38 commits into from Oct 1, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Sep 25, 2019

Resolves #5288
Resolves #5312
Impact: major
Type: feature

Changes

  • New included job-queue plugin. It's a port of @reactioncommerce/job-queue, without any Meteor dependencies. Although it attempts to keep the same API, the loss of fibers means that functions that used to be magically synchronous now need to be awaited.
  • Fully demeteorizes the sitemap-generator plugin. Updates its client code to use GraphQL. Updates refresh period setting to use the new shopSettings API.
  • Removed the outdated exchange rate updater job. Setting the rates some other way should still work if you have custom plugins that rely on them.
  • New APIs, more details below

New background jobs API

The background jobs API is abstracted onto context.backgroundJobs so that you do not need to import from outside your own plugin to use it. (It is theoretically possible to replace the included handling with your own background job solution, if you simulate the same API exactly. This isn't tested or officially supported yet.)

To register a worker, typically done in your plugin startup function:

  const workerInstance = await context.backgroundJobs.addWorker({
    type: "someUniqueJobType",
    async worker(job) {
        await doTheWork(job.data); // Whatever function you create that does the task
        job.done("Done message");
        // If anything throws, it will automatically call job.fail(errorMessage), but you
        // could also call job.fail yourself to provide better failure details.
    }
  });

To schedule a job for that worker, either on startup or anywhere you have context:

  const job = await context.backgroundJobs.scheduleJob({
    type: "someUniqueJobType",
    data: {}, // any data your worker needs to perform the work
    // Retry is optional
    retry: {
      retries: 5,
      wait: 60000,
      backoff: "exponential"
    },
    // Priority is optional
    priority: "normal",
    // Schedule is optional if you just need to run it once.
    // Set to any text that later.js can parse.
    schedule: "every day",
    // Set cancelRepeats to true if you want to cancel all other pending jobs with the same type
    cancelRepeats: true
  });

To cancel all jobs that match a certain filter:

await context.backgroundJobs.cancelJobs({
  type: "foo",
  data: { shopId }
});

To get a job by ID:

const job = await context.backgroundJobs.getJob(jobId);

Background job cleanup

Plugins can now request cleanup of their finished/old jobs through registerPlugin.

app.registerPlugin({
  // ...
  backgroundJobs: {
      cleanup: [
        { type: "saveImage/local", purgeAfterDays: 7 },
        { type: "saveImage/remote", purgeAfterDays: 7 }
      ]
    }
});

contextAdditions

A plugin can now add properties to context using the contextAdditions option when calling registerPlugin. They are added before any startup functions are run.

app.registerPlugin({
  // ...
  contextAdditions: {
    something: "wicked"
  }
});
// in startup fn or anywhere you have context
console.log(context.something); // "wicked"

Breaking changes

Just one: There are no longer background jobs that keep the exchange rates up to date. Stock Reaction does not use this feature any more. If you are relying on it, everything should continue to work, but you will need to devise your own way to periodically fetch exchange rates and update the rate field.

Despite efforts to keep the API the same, the loss of fibers means that functions that used to be magically synchronous now need to be awaited. In many cases existing code would just work, but to be safe, I did not remove the non-Meteor job-collection files. Custom plugins using background jobs should switch to the new context.backgroundJobs API sometime before the 3.0 release, at which time the old job-queue package will be removed.

Testing

Generally need to test anything that relies on job queue. Do all of these:

  • Set up email sending and do anything that causes an email to be sent, such as placing an order or registering a new account. Verify you get the email. On Email settings page, find the email row and click retry icon. Verify you get the email again.
  • Upload image files
  • On shop settings page, test everything about the "Sitemaps" card. You should see logging in server console as you change the refresh period or do a manual refresh. When you view the sitemap, you should see XML.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Demeteorized version of @reactioncommerce/job-queue

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Exchange rate feature isn't currently used. Setting the rates
some other way should still work if you use it.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
It is no longer used by built-in plugins. The file remains
for any custom plugins that import it, but they should
be updated to use the non-Meteor job-queue.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed aldeed self-assigned this Sep 25, 2019
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

A few comments left inline.

A couple issues as well:

  1. When I tried to re-send an email to myself, it never sent (or hasn't yet). What's the default time set as for retrying jobs? It's been about 20 minutes and the email still hasn't been resent. Is it one hour, as defaulted here? https://github.com/reactioncommerce/reaction/pull/5580/files#diff-80f1e8e394b63511ca10855091209060R7

  2. After uploading an image in the shop settings, and trying to make it my shop logo, I get the following errors (although the image does become my shop logo):

Client Error:

When uploading a shop image, and then trying to to make my shop logo, I get the following GQL error:

Uncaught (in promise) Error: GraphQL error: Cannot read property 'startsWith' of null
    at new ApolloError (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:322470)
    at modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:324506
    at Object.next (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:328461)
    at notifySubscription (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:328283)
    at onNotify (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:328318)
    at SubscriptionObserver.next (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:328372)
    at modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:323473
    at Set.forEach (<anonymous>)
    at Object.next (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:323472)
    at notifySubscription (modules.js?hash=ecdc6fd4a2310fc4aa30a0b7d58c66a7bdb6dd87:328283)

Server:

21:45:05.478Z ERROR Reaction: Cannot read property 'startsWith' of null (errorId=ck0zsre8500005moagtok13qa)
  path: [
    "shop",
    "brandAssets"
  ]
  --
  stack: TypeError: Cannot read property 'startsWith' of null
      at getAbsoluteUrl (imports/plugins/core/core/server/util/getAbsoluteUrl.js:10:7)
      at Object.context.getAbsoluteUrl.path [as getAbsoluteUrl] (imports/node-app/core/util/buildContext.js:45:38)
      at Promise.asyncApply (imports/plugins/core/shop/server/resolvers/Shop/brandAssets.js:28:18)
      at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40

mutations: {},
queries: {},
rootUrl: ROOT_URL
},
graphQL: {
graphiql: true
Copy link
Member

Choose a reason for hiding this comment

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

This will remove the localhost:3000/graphiql app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is no longer used. It uses env variables to determine whether to serve the playground UI.

@@ -49,11 +51,13 @@ export default async function registerPlugins(app) {
* CORE
* (Core plugin needs Media collection, so files plugin must be first)
Copy link
Member

Choose a reason for hiding this comment

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

We should either update this comment to say Job must run first, or reorder the way it's run to make files first again.

@@ -1,95 +0,0 @@
import _ from "lodash";
Copy link
Member

Choose a reason for hiding this comment

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

Do we no longer need to fetch / flush currency rates? I didn't see this re-added anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use the rates anywhere so I just removed this. They were complicated to demeteorize so it didn't seem worth the time for something unused. All other rate/conversion code is still there so someone could make their own job to fetch and set rates if needed. I did put this in the PR description, but I guess I should list it as a breaking change.

import Reaction from "/imports/plugins/core/core/server/Reaction";
import ReactionError from "@reactioncommerce/reaction-error";
import getGraphQLContextInMeteorMethod from "/imports/plugins/core/graphql/server/getGraphQLContextInMeteorMethod";
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming this is OK to put here, and no other way to get context, as we were trying to remove it in all the PR's I was working on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we'll need to move this Meteor method to GraphQL like the others after this is merged, but I didn't want to make this PR any longer.

const useStyles = makeStyles(() => ({
card: {
// Without this, the Select dropdown menu gets cut off at the bottom of the card
overflow: "visible"
Copy link
Member

Choose a reason for hiding this comment

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

@catalystpod (@mikemurray @machikoyasuda @willopez @rymorgan @cassytaylor)

See @aldeed's comment here. We've noticed this getting cut off in a few places, the Origin country of the Product admin comes to mind as one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I first tried using the Catalyst Select, but that one does not accept a value prop with the current value so it's kind of useless for an update form such as this. I will submit a catalyst issue about that when I get a chance.


return (
<Card className={classes.card}>
<CardHeader title={i18next.t("SitemapSettings.cardTitle")} />
Copy link
Member

Choose a reason for hiding this comment

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

Style consistency question: Should these translation keys be camelCased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied what I had done in inventory, which is exactly matching the component name. I don't remember why I did it that way, though. It doesn't matter much to me, but I will follow a consistency rule if someone wants to make one.

}

// This function soaks up num callbacks, by default returning the disjunction of Boolean results
// or returning on first error.... Reduce function causes different reduce behavior, such as concatenation
Copy link
Member

Choose a reason for hiding this comment

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

Only three . in the ellipses

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

I've seen a huge slowdown in the app when running this branch. I've switched to a few other branches and back to this one, and can confirm I only see it on this branch.

Everything from login, to modules loading (sitemap settings, product editing UI blocks, payment settings). Publishing a product takes 2-3 seconds for the Toast to confirm.

I'm not seeing any error messages or anything, but something seems to be going on in the background that's slowing everything down.

Even the CI build of the tests seems to be very, very slow, it's been stuck on the integration tests for 5 hours as of the time writing this comment.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Currently only used for integration tests

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

👍 Issues I was seeing before see to have been resolved.

Startup seems a tad slower, nothing that I would consider blocking, just making a note.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed aldeed merged commit 1fa0b9f into develop Oct 1, 2019
@aldeed aldeed deleted the feat-aldeed-non-meteor-jobs branch October 1, 2019 17:43
@kieckhafer kieckhafer mentioned this pull request Oct 2, 2019
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

2 participants