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

Symphosize improvements #84

Merged
merged 2 commits into from
Nov 16, 2016

Conversation

Symphosize
Copy link

This includes our changes to the Queue plugin. There are two main changes included with this pull request.

  1. We added the concept of priority to queued tasks. By default all tasks will have a priority of 5. But you can specify a lower number like 3 and this will place the task in front of those with 5.

$this->QueuedTask->nextPriority(2)->createJob('YourSuperCoolTask', [ 'some_id' => '12345', ]);

  1. We have had cases where a task that is working with a remote api fails because of exceptions raised when the API is down. We should be catching these exceptions in the task and handle failure there. But this is a fail safe. We added a try catch block that will log any exception making it up to the runworker method. then mark the task as failed and move on with out causing the worker to die.

I am open to suggestion on how we can make this a useful contribution to the plugin. Any feedback is welcome.

@Symphosize
Copy link
Author

I see the tests are failing because of our missing priority column, We have included a cakeDC migration, but I don't think these were used by this repo. Should I also include a schema snapshot?

@dereuromark
Copy link
Owner

I like the changes.
The tests use fixtures. Those need updating.

@Symphosize
Copy link
Author

I'll update the fixtures tomorrow,

I'll also push a slight change to our exception catcher to use \Exception $e instead of just Exception $e I just learned that when code is name spaced plain old Exception won't catch :-( I will also take a look at the tests and see what I can do about testing the nextPriority method

@dereuromark
Copy link
Owner

But this 2.x code is not namespaced, thus it should not matter.

@Symphosize
Copy link
Author

However the libs we use are often namespaced. For example I am having trouble with the Google API throwing and Error, and using catch( Exception $e ) {} fails to catch the error... it has to be catch( \Exception $e ) {} What I don't know is if I need both catch blocks or if the namespaced version will work as a catch all for any exception bubbling up to the worker. Just trying to put up a firewall so that our workers don't fall over in the middle of the night.

* @param string $direction Direction of migration process (up or down)
* @return bool Should process continue
*/
public function after($direction) {
Copy link
Owner

Choose a reason for hiding this comment

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

Would be great if you could run the sniffer over this to auto-fix the comment indentation.

@dereuromark dereuromark added this to the 2.x milestone Apr 8, 2016
@Symphosize
Copy link
Author

Just a note, that I am still planning on making these changes. Will need a couple more days

@dereuromark
Copy link
Owner

Cool

];
$this->_next_priority = 5; // reset to default;
Copy link
Owner

Choose a reason for hiding this comment

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

_nextPriority

@dereuromark dereuromark merged commit 71273e1 into dereuromark:2.x Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants