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

Add Job status update plugin (php-resque way) #335

Closed
wants to merge 3 commits into from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Apr 1, 2020

This adds :status key updating logic in redis, so that status monitoring can be used with php-resque.

// Create worker object
const worker = new Worker(...);

// Attach WorkerJobStatusPlugin events to worker
new WorkerJobStatusPlugin(worker);

Refs:

@glensc
Copy link
Contributor Author

glensc commented Apr 2, 2020

perhaps this should be named as WorkerJobStatusSubscriber implementing Observer pattern:

worker.addSubscriber(new WorkerJobStatusSubscriber)

where Subscriber provides method called getSubscribedEvents

WorkerJobStatusSubscriber.getSubscribedEvents() {
  return [
    'eventName' : callable,
    'start': this.onStart,
   ]
}

as this particular subscriber needs access to worker, it could be passed in constructor:

worker.addSubscriber(new WorkerJobStatusSubscriber(worker))

but more flexible solution would be if it captures it on 'start' event:

onStart(worker) => {
  this.worker = worker;
}

I come from PHP background, so this is designed by symfony/event-dispatcher:

@evantahler WDYT?

@evantahler
Copy link
Member

great start!

Do you need help writing a test for this (and getting the linter to pass)? Also, please add an example for folks to see, and update the readme.

@glensc
Copy link
Contributor Author

glensc commented Apr 4, 2020

@evantahler not sure what you mean by "this"?

as for example, the pull request commit contains an example on its own:

you mean you want example in pull request body?

ps: I'm using this "plugin" successfully with jobs submitted from php-resque

@glensc glensc changed the title Add Job status plugin Add Job status update plugin (php-resque way) Apr 4, 2020
@glensc
Copy link
Contributor Author

glensc commented Apr 4, 2020

@evantahler started the addSubscriber development:

@evantahler
Copy link
Member

I think that I'm having trouble understanding the eventual usage / API you have in mind for this (and #337). Could you please start with an example that shows how you might use this? I think what I'm stuck on is that to me is seems that the person requesting the job's status is likely not on the same server as the worker... so they won't be able to subscribe to events on the worker doing the job.

I'm sure this is going to be awesome, but I'm just missing something... Thank you!

@glensc
Copy link
Contributor Author

glensc commented Apr 5, 2020

did you see updated PR body? this integration is inserted to worker. client is php-resque.

#337 is to write plugins like that, and attach bulk of listeners with single line. the listeners are defined in the class. class knows what it wants to listen, and provides handlers for those.

please see how Symfony event listeners are done, should help to understand the principle:

@evantahler
Copy link
Member

Thank you for the example, I found that very helpful!

Reading https://github.com/quirkey/resque-status, it seems that what you are trying to accomplish is to store data about jobs that lives past their completion. If that's the case, I'm still curious why the normal plugin lifecycle hooks beforePerform and afterPerform aren't enough to store this new data? I don't think afterPerform will be invoked both on success and failure, and in the case of failure, you will have the error on worker.error

It also looks like jobUUIDs would be useful, and we can certainly implement that in an agnostic way at enqueue time, but perhaps that's not needed?

Also, I imagine you will need a TTL on your new data objects (it looks like https://github.com/quirkey/resque-status has a default of 24h)

If you'd like to chat more realtime, please join us on the Actionhero Slack and we can make a dedicated room for this topic https://slack.actionherojs.com

@glensc
Copy link
Contributor Author

glensc commented Apr 8, 2020

I'm not using quirkey/resque-status but resque/php-resque, and as the status handling is non-standard, they both do their own way.

@glensc
Copy link
Contributor Author

glensc commented Apr 27, 2020

looks like adding multiWorker support for this plugin is very complicated. mostly because there's no API to contain the connection object. so for single worker plugin was just operating on worker.connection property which multiworker does not have.

also, design-wise, seems multi worker creates workers with passing connection details to the worker, so each worker creates their own connection to database, while it could be optimized of connection object is passed, without need to create more connections to database for each worker.

@evantahler
Copy link
Member

Yeah, I imagine so. Again, please join us on slack.actionherojs.com if you want to talk more about your use case. Your implementation is rather far-removed from the plugin interfaces we have...

Regarding sharing a redis connection, you can do that today! It works for multiWorker, workers, schedulers... all the process types. Just pass in a redis client rather than connection options:

const connectionDetails = {
pkg: "ioredis",
host: "127.0.0.1",
password: null,
port: 6379,
database: 0,
// namespace: 'resque',
// looping: true,
// options: {password: 'abc'},
};
// OR
// var ioredis = require('ioredis');
// connectionDetails = { redis: new ioredis() };

@glensc
Copy link
Contributor Author

glensc commented Apr 27, 2020

yea, doing new ioredis myself removes the abstraction that you have in place in Connection class.

so, for now I'll leave it as is, I can survive 10 Redis connections per node.

@glensc
Copy link
Contributor Author

glensc commented Apr 28, 2020

pushed changes to support multiworker.

I noticed the function signatures do not match, i.e job is typed differently on methods this plugin listens to.

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