- Download Diff

Summary:

[Spwn] add Resque job dispatcher to lunr

Review Request #25 — Created Dec. 18, 2012 and submitted Jan. 11, 2013, 8:16 a.m.

Information
Owner:	olivier
Repository:	Lunr
Branch:	https://github.com/owizen/lunr/tree/dispatcher
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Add a backgroundjob dispatcher to lunr for Resque

Testing Done

Unit test, 100% covering

Issues

1
0
28
1
All Issues: 30

Description	From	Last Updated
Dependency Injection	pprkut	Dec. 19, 2012, 11:31 a.m.
missing destruction of $resque	pprkut	Dec. 19, 2012, 11:31 a.m.
I'd rather have $queue a default queue than NULL	pprkut	Dec. 19, 2012, 11:32 a.m.
Two methods don't really warrant a new test file. Just have one test file for the whole class, same as ...	pprkut	Dec. 19, 2012, 11:32 a.m.
That's a ReflectionClass	pprkut	Dec. 19, 2012, 11:32 a.m.
I don't see why this is necessary. The Mock should have already replaced this with an empty method block...	pprkut	Dec. 19, 2012, 12:01 p.m.
that's a bit long of a variable name, no?	pprkut	Dec. 19, 2012, 11:32 a.m.
That needs better documentation	pprkut	Dec. 19, 2012, 3:56 p.m.
mention it's optional	pprkut	Dec. 19, 2012, 3:57 p.m.
erm....	pprkut	Dec. 19, 2012, 3:57 p.m.
what would that be in the case of a CliBackgroundJob?	pprkut	Dec. 19, 2012, 5:14 p.m.
This signature is incompatible to: public static function enqueue($queue, $class, $args = null, $trackStatus = false)	pprkut	Dec. 19, 2012, 5:14 p.m.
don't pass the mock without keeping a reference	pprkut	Dec. 19, 2012, 5:14 p.m.
reuse the reference	pprkut	Dec. 19, 2012, 5:14 p.m.
reuse the existing reference	pprkut	Dec. 19, 2012, 5:14 p.m.
unused	pprkut	Dec. 19, 2012, 5:25 p.m.
I'm not sure this is the correct naming here. Cli won't have that functionality, but for Resque it's wrong since ...	pprkut	Dec. 19, 2012, 6:11 p.m.
not tested	pprkut	Dec. 19, 2012, 6:11 p.m.
wrong copyright	pprkut	Dec. 19, 2012, 5:26 p.m.
still invalid signature	pprkut	Dec. 19, 2012, 5:28 p.m.
Dude, this is still an incompatible method signature... How often do I have to mention this?	pprkut	Dec. 21, 2012, 6 p.m.
Coding style	pprkut	Jan. 9, 2013, 2:54 p.m.
wrong namespace	pprkut	Jan. 9, 2013, 2:21 p.m.
out of date	pprkut	Jan. 9, 2013, 2:22 p.m.
I don't like that. Please do that explicitely in the tests	pprkut	Jan. 9, 2013, 2:53 p.m.
what's the difference between these that you have to test all of them?	pprkut	Jan. 10, 2013, 5:31 p.m.
assertFalse....	pprkut	Jan. 10, 2013, 5:31 p.m.
assertSame	pprkut	Jan. 10, 2013, 5:31 p.m.
assertNotSame	pprkut	Jan. 10, 2013, 5:31 p.m.
what's this?	pprkut	Jan. 10, 2013, 5:20 p.m.

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 1)
Show all issues
```
Dependency Injection
```
1. olivier Dec. 19, 2012, 10:12 a.m.
  OK
system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 1)
Show all issues
```
missing destruction of $resque
```
1. olivier Dec. 19, 2012, 10:12 a.m.
  OK

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 1)

Show all issues

I'd rather have $queue a default queue than NULL

olivier Dec. 19, 2012, 10:12 a.m.

Yep good Idea, you can tell the workers to work on all available

tests/system/libraries/spwn/class.resquejobdispatcher.dispatch.test.php (Diff revision 1)

Show all issues

Two methods don't really warrant a new test file. Just have one test file for the whole class, same as we do for various other small classes.

olivier Dec. 19, 2012, 10:12 a.m.
```
OK
```

tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
Show all issues
```
That's a ReflectionClass
```
1. olivier Dec. 19, 2012, 10:12 a.m.
  OK
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
Show all issues
```
I don't see why this is necessary. The Mock should have already replaced this with an empty method block...
```
1. olivier Dec. 19, 2012, 12:20 p.m.
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
Show all issues
```
that's a bit long of a variable name, no?
```
1. olivier Dec. 19, 2012, 10:12 a.m.
  rename to dispatcher then

Change Summary:

change made according to review.

Introduce mock class for resque

Diff:

Revision 2 (+229)

Show changes

	system/libraries/spwn/class.resquejobdispatcher.inc.php
	tests/phpunit.xml
	tests/mocks/libraries/spwn/class.resque.mock.php
	tests/system/libraries/spwn/class.resquejobdispatcher.test.php

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 2)
Show all issues
```
That needs better documentation
```
system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 2)
Show all issues
```
mention it's optional
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 2)
Show all issues
```
erm....
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 2)
Show all issues
```
what would that be in the case of a CliBackgroundJob?
```

tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 2)

Show all issues

This signature is incompatible to:

public static function enqueue($queue, $class, $args = null, $trackStatus = false)

tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 2)
Show all issues
```
don't pass the mock without keeping a reference
```
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 2)
Show all issues
```
reuse the reference
```
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 2)
Show all issues
```
reuse the existing reference
```

Change Summary:

Change made according to review.
Method get_result added in interface for getting the result of the dispatching.

Diff:

Revision 3 (+272)

Show changes

	system/libraries/spwn/class.resquejobdispatcher.inc.php
	system/libraries/spwn/interface.jobdispatcher.inc.php
	tests/phpunit.xml
	tests/mocks/libraries/spwn/class.resque.mock.php
	tests/system/libraries/spwn/class.resquejobdispatcher.test.php

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 3)
Show all issues
```
unused
```

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 3)

Show all issues

I'm not sure this is the correct naming here. Cli won't have that functionality, but for Resque it's wrong since you just get the Job ID not the result.

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 3)
Show all issues
```
not tested
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 3)
Show all issues
```
wrong copyright
```

tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 3)

Show all issues

still invalid signature

Change Summary:

rename get_result to get_job_id in interface and resque job dispatcher

unit test updated to cover the implementation of that method

Diff:

Revision 4 (+292)

Show changes

	system/libraries/spwn/class.resquejobdispatcher.inc.php
	system/libraries/spwn/interface.jobdispatcher.inc.php
	tests/phpunit.xml
	tests/mocks/libraries/spwn/class.resque.mock.php
	tests/system/libraries/spwn/class.resquejobdispatcher.test.php

Fix it!

tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 4)

Show all issues

Dude, this is still an incompatible method signature...
How often do I have to mention this?

olivier Jan. 3, 2013, 10:35 a.m.
```
Oh ca va hein!
```

Change Summary:

Changed resque mock enqueue method signature.

Diff:

Revision 5 (+301)

Show changes

	system/libraries/dataaccess/class.databasedmlquerybuilder.inc.php
	system/libraries/spwn/class.resquejobdispatcher.inc.php
	system/libraries/spwn/interface.jobdispatcher.inc.php
	tests/phpunit.xml
	tests/mocks/libraries/spwn/class.resque.mock.php
	tests/system/libraries/spwn/class.resquejobdispatcher.test.php

Change Summary:

Switch to psr-0 specifications.

Diff:

Revision 6 (+233)

Show changes

	src/Lunr/Spwn/JobDispatcherInterface.php
	src/Lunr/Spwn/ResqueJobDispatcher.php
	src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php
	tests/phpunit.xml

src/Lunr/Spwn/ResqueJobDispatcher.php (Diff revision 6)
Show all issues
```
Coding style
```

src/Lunr/Spwn/ResqueJobDispatcher.php (Diff revision 6)

Thought: How about making those setters?
Most of the time you probably dispatch similar jobs in a loop, then setting the queue and track status once makes for an easier interface than having to pass it all the time. What do you think?

olivier Jan. 9, 2013, 1:25 p.m.

The idea is good, for a CLI job dispatcher, the queue will be the script name.
For the trackStatus, it could give the possibility to get the exit code of the process for CLI
the get_job_id should then be rename to a more global name covering all the possibilities.

pprkut Jan. 9, 2013, 2:46 p.m.

That the queue will be the script name is an assumption I would not make.
Using trackstatus for the cli exit code seems like a sensible choice, however I do not follow with get_job_id. What do you mean exactly?

olivier Jan. 9, 2013, 3:23 p.m.

Right for the queue, it is not the job and the job is the script, I talked too fast.

For the get_job_id, that method should return the resque token or the exit code returned by CLI
but by thinking back to this (was already discussed I think), having the exit code is not possible for background job...

I made a little test and it doesn't work, BUT it seems that the exec function allows us to get the output of the process as an array
and it works with background tasks.

pprkut Jan. 9, 2013, 6:25 p.m.

Right, but we also said that you don't want the output of a cli background task, since waiting for the output basically doesn't make it run in the background anymore.
Additionally, the job id is not the equivalent of the exit code, it would be the process id. The exit code would be equivalent to the return value.

src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revision 6)
Show all issues
```
wrong namespace
```
1. olivier Jan. 9, 2013, 1:25 p.m.
  Oops
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revision 6)
Show all issues
```
out of date
```
1. olivier Jan. 9, 2013, 1:25 p.m.
  Will be corrected

src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revision 6)

Show all issues

I don't like that. Please do that explicitely in the tests

olivier Jan. 9, 2013, 1:25 p.m.

You mean moving it to the test case instead of the setUp method?
Will be done so then

pprkut Jan. 9, 2013, 2:46 p.m.

Yes. There are some cases where $this->any() in setUp is preferable. I'm not sure that's the case here though.

Change Summary:

Correction made from review, code style check passed.

Remains the setters question for trackStatus and queue

Diff:

Revision 7 (+239)

Show changes

	src/Lunr/Spwn/JobDispatcherInterface.php
	src/Lunr/Spwn/ResqueJobDispatcher.php
	src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php
	tests/phpunit.xml

Change Summary:

add setters for track status and queue properties, unit tests updated, code styleok

once this is fixed, the setters could be part of the job dispatcher interface

Diff:

Revision 8 (+471)

Show changes

	src/Lunr/Spwn/JobDispatcherInterface.php
	src/Lunr/Spwn/ResqueJobDispatcher.php
	src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php
	tests/phpunit.xml

src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)

Show all issues

what's the difference between these that you have to test all of them?

src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)
Show all issues
```
assertFalse....
```
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)
Show all issues
```
assertSame
```
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)
Show all issues
```
assertNotSame
```

tests/phpunit.xml (Diff revisions 7 - 8)

Show all issues

what's this?

Change Summary:

fix made in unit tests due to review

Diff:

Revision 9 (+455)

Show changes

	src/Lunr/Spwn/JobDispatcherInterface.php
	src/Lunr/Spwn/ResqueJobDispatcher.php
	src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php
	tests/phpunit.xml

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Spwn] add Resque job dispatcher to lunr

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: