This change has been marked as submitted.

Describe the submission (optional):

Merged into master

Reviews
Diff

- Download Diff

Summary:

[Spwn] add Resque job dispatcher to lunr

Review Request #25 — Created Dec. 18, 2012 and submitted 12 years, 4 months ago

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	12 years, 4 months ago
missing destruction of $resque	pprkut	12 years, 4 months ago
I'd rather have $queue a default queue than NULL	pprkut	12 years, 4 months ago
Two methods don't really warrant a new test file. Just have one test file for the whole class, same as ...	pprkut	12 years, 4 months ago
That's a ReflectionClass	pprkut	12 years, 4 months ago
I don't see why this is necessary. The Mock should have already replaced this with an empty method block...	pprkut	12 years, 4 months ago
that's a bit long of a variable name, no?	pprkut	12 years, 4 months ago
That needs better documentation	pprkut	12 years, 4 months ago
mention it's optional	pprkut	12 years, 4 months ago
erm....	pprkut	12 years, 4 months ago
what would that be in the case of a CliBackgroundJob?	pprkut	12 years, 4 months ago
This signature is incompatible to: public static function enqueue($queue, $class, $args = null, $trackStatus = false)	pprkut	12 years, 4 months ago
don't pass the mock without keeping a reference	pprkut	12 years, 4 months ago
reuse the reference	pprkut	12 years, 4 months ago
reuse the existing reference	pprkut	12 years, 4 months ago
unused	pprkut	12 years, 4 months ago
I'm not sure this is the correct naming here. Cli won't have that functionality, but for Resque it's wrong since ...	pprkut	12 years, 4 months ago
not tested	pprkut	12 years, 4 months ago
wrong copyright	pprkut	12 years, 4 months ago
still invalid signature	pprkut	12 years, 4 months ago
Dude, this is still an incompatible method signature... How often do I have to mention this?	pprkut	12 years, 4 months ago
Coding style	pprkut	12 years, 4 months ago
wrong namespace	pprkut	12 years, 4 months ago
out of date	pprkut	12 years, 4 months ago
I don't like that. Please do that explicitely in the tests	pprkut	12 years, 4 months ago
what's the difference between these that you have to test all of them?	pprkut	12 years, 4 months ago
assertFalse....	pprkut	12 years, 4 months ago
assertSame	pprkut	12 years, 4 months ago
assertNotSame	pprkut	12 years, 4 months ago
what's this?	pprkut	12 years, 4 months ago

system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 1)
The issue has been resolved. Show all issues
```
Dependency Injection
```
1. olivier 12 years, 4 months ago
  OK
system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 1)
The issue has been resolved. Show all issues
```
missing destruction of $resque
```
1. olivier 12 years, 4 months ago
  OK

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

The issue has been resolved. Show all issues

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

olivier 12 years, 4 months ago

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)

The issue has been resolved. 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 12 years, 4 months ago
```
OK
```

tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
The issue has been resolved. Show all issues
```
That's a ReflectionClass
```
1. olivier 12 years, 4 months ago
  OK
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
The issue has been resolved. 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 12 years, 4 months ago
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 1)
The issue has been resolved. Show all issues
```
that's a bit long of a variable name, no?
```
1. olivier 12 years, 4 months ago
  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)
The issue has been resolved. Show all issues
```
That needs better documentation
```
system/libraries/spwn/class.resquejobdispatcher.inc.php (Diff revision 2)
The issue has been resolved. Show all issues
```
mention it's optional
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 2)
The issue has been resolved. Show all issues
```
erm....
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 2)
The issue has been resolved. Show all issues
```
what would that be in the case of a CliBackgroundJob?
```

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
don't pass the mock without keeping a reference
```
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 2)
The issue has been resolved. Show all issues
```
reuse the reference
```
tests/system/libraries/spwn/class.resquejobdispatcher.test.php (Diff revision 2)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
unused
```

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
not tested
```
tests/mocks/libraries/spwn/class.resque.mock.php (Diff revision 3)
The issue has been resolved. Show all issues
```
wrong copyright
```

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

The issue has been resolved. 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)
An issue was opened. Show all issues
```
Dude, this is still an incompatible method signature...
How often do I have to mention this?
```
1. olivier 12 years, 4 months ago
  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)
The issue has been resolved. 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 12 years, 4 months ago

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 12 years, 4 months ago

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 12 years, 4 months ago

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 12 years, 4 months ago

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)
The issue has been resolved. Show all issues
```
wrong namespace
```
1. olivier 12 years, 4 months ago
  Oops
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revision 6)
The issue has been resolved. Show all issues
```
out of date
```
1. olivier 12 years, 4 months ago
  Will be corrected

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

The issue has been resolved. Show all issues

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

olivier 12 years, 4 months ago

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

pprkut 12 years, 4 months ago

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)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
assertFalse....
```
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)
The issue has been resolved. Show all issues
```
assertSame
```
src/Lunr/Spwn/Tests/ResqueJobDispatcherTest.php (Diff revisions 7 - 8)
The issue has been resolved. Show all issues
```
assertNotSame
```

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

The issue has been dropped. 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

This change has been marked as submitted.

[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: