search

apinstein / jqjobs

A job queue engine for PHP.
32 stars 5 forks source link

Worker for multiple queues #32

Open apinstein opened 8 years ago

apinstein commented 8 years ago

This change is Reviewable

apinstein commented 8 years ago

@leftdevel can you check out this early PR to see if you have any architectural concerns w/doing this? I was trying to get ahead of work required to finish our queue tuning.

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

_Comments from Reviewable_

apinstein commented 8 years ago

I made some tests to mess w/POC for this....

[1 git@workerForMultipleQueues ]:☹ 1> ./vendor/bin/phpunit  --testdox --filter '/next.*/i' test/JQStore_ArrayTest.php                 
PHPUnit 3.5.0beta1-1384-g636c21a by Sebastian Bergmann.

Configuration read from /Users/apinstein/dev/jqjobs/phpunit.xml

JQStore_Array
 [X] Test JQStore->next(NULL) finds next job available on all queues
 [X] Test JQStore->next('a') finds next job available on queue 'a' only
 [X] Test JQStore->next('a,b') finds next job available on queues 'a' or 'b'

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

_Comments from Reviewable_

apinstein commented 8 years ago

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file): The other arch q is -- read my README changes about queue / worker mgmt, and see if it makes sense that we should allow this. I think any argument that extends to "allow singly-named queues in workers" extends to "multiply-named queues" but I just want to be sure I'm not being TOO clever here in allowing emergent architectures of partitioning work across queues....

a discussion (no related file): I did the 'a,b' architecture for indicating multiple queues b/c it has to have a textual interface for the CLI invocation. However, that might be allowing bad things to leak through; maybe the ',' support should be in the JQWorker constructor only, and internally always use it as a STRING | Array of Strings union type? That's one of my main architectural q's.

Comments from Reviewable

apinstein commented 8 years ago

@woofyman99 here's the PR for the JQJobs workers for multiple queues

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

leftdevel commented 8 years ago

Reviewed 6 of 6 files at r1, 2 of 2 files at r2. Review status: all files reviewed at latest revision, 6 unresolved discussions.

a discussion (no related file):

Previously, apinstein (Alan Pinstein) wrote… > I did the 'a,b' architecture for indicating multiple queues b/c it has to have a textual interface for the CLI invocation. However, that might be allowing bad things to leak through; maybe the ',' support should be in the JQWorker constructor only, and internally always use it as a STRING | Array of Strings union type? That's one of my main architectural q's. >

Left a comment about this on the matchesQueueName method.

a discussion (no related file):

Previously, apinstein (Alan Pinstein) wrote… > The other arch q is -- read my README changes about queue / worker mgmt, and see if it makes sense that we should allow this. I _think_ any argument that extends to "allow singly-named queues in workers" extends to "multiply-named queues" but I just want to be sure I'm not being TOO clever here in allowing emergent architectures of partitioning work across queues.... >

Sounds sane to me, though maybe I'm not able to see the full picture (how these changes may affect an existing implementation; what would be the concerns if someone had worked with a single queue, and now wants to move to multiple queues, what should that person expect in terms of monitoring, debugging, etc, just food for thought).

_src/JQJobs/JQManagedJob.php, line 362 [r2] (raw file):_

    }

    public function matchesQueueNameFilter($filterBy)

This method is not documented, and the $filterBy format makes it harder to guess what to pass. Maybe instead of passing a comma separated string, you can support either a string (one single queue), or an array for multiple queues (this is a pattern I've seen many times):

$queueFilters = [];

if (is_string($filterBy) {
    $queueFilters = [$filterBy];
}
// otherwise if's an array, there's no need to explode by ','.

src/JQJobs/JQManagedJob.php, line 364 [r2] (raw file):

    public function matchesQueueNameFilter($filterBy)
    {
        if ($filterBy === NULL or $filterBy === JQManagedJob::QUEUE_ANY)

I think we should standardize the use of 'or' vs '||'. I do prefer the pipes since they have less 'aha' things ('and' behaves similar). For instance:

$var = false || true;
var_dump($var === true); // this would be true.

$var = false or true;
var_dump($var === true); // this would be false, since $var value is false, since it got immediately assigned to $var; 'true' got evaluated but not assigned.
// in order to fix this we'd need to wrap it between parenthesis:
$var = (false or true);
var_dump($var === true); // dumps true.

_test/JQStore_AllTest.php, line 257 [r2] (raw file):_

    }

    private function setupOneJobInThreeQueues(JQStore $q)

I'd pass the queue number as an arg, and prob the job count across the queues too. Maybe it's not that important here because this is a test, but still feels pretty rigid to me.

_test/JQStore_AllTest.php, line 284 [r2] (raw file):_

        $w->start();

        $this->assertEquals(3, $w->jobsProcessed());

.... If you pass the queue count as an arg, and the jobs count across the queues, as in $this->setupJobsInQueues($q, $queueCount = 3, $jobsPerQueue = 1);,

then this assert would read:

$totalJobs = $queueCount * $jobsPerQueue;
$this->assertEquals($totalJobs, $w->jobsProcessed());

The main benefit is that if for some reason we later need to assert 4 total jobs, then we'll need to also edit the setupOneJobInThreeQueues() code, and refactor all other tests that make use of this method.

Also that the '3' in the assert seems kind of magic since it's correlated to a method that happens to insert 3 jobs.

But I think it mainly depends on how likely these tests will change in future. If it's very unlikely then I would leave it as it is. Thoughts?

Comments from Reviewable

apinstein commented 8 years ago

Review status: all files reviewed at latest revision, 6 unresolved discussions.

a discussion (no related file):

Previously, leftdevel (Oscar Balladares M.) wrote… > Sounds sane to me, though maybe I'm not able to see the full picture (how these changes may affect an existing implementation; what would be the concerns if someone had worked with a single queue, and now wants to move to multiple queues, what should that person expect in terms of monitoring, debugging, etc, just food for thought). >

I'd expect no BC issues; the new feature wouldn't change how existing things are flowing.

It is always a client-of-jqjobs concern to reason about how queues are organized. This is classic queueing theory and operational management. Tons of academic info about it (I studied it in school, see https://en.wikipedia.org/wiki/Queueing_theory )

Should we link to that in the readme as a guide for people to grok how to coordinate the queues?

a discussion (no related file):

Previously, leftdevel (Oscar Balladares M.) wrote… > Left a comment about this on the `matchesQueueName` method. >

I'd like more info specifically here on the architecture of STRING vs internal types. That's the main open q I have :)

_src/JQJobs/JQManagedJob.php, line 362 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > This method is not documented, and the $filterBy format makes it harder to guess what to pass. Maybe instead of passing a comma separated string, you can support either a string (one single queue), or an array for multiple queues (this is a pattern I've seen many times): > > ``` > $queueFilters = []; > > if (is_string($filterBy) { > $queueFilters = [$filterBy]; > } > // otherwise if's an array, there's no need to explode by ','. > ``` > >

this is just a PoC -- it was purposefully not doc'd. Sorry if I didn't make that clear. I am just trying to figure out if the main architectural changes make sense -- I only did a small reference implementation to reason about how hard it would be to technically implement the feature.

I'm more interested in your opinion on where STRING 'a,b' format should be accepted, and where we force it to more rigorous types.

src/JQJobs/JQManagedJob.php, line 364 [r2] (raw file):

Previously, leftdevel (Oscar Balladares M.) wrote… > I think we should standardize the use of 'or' vs '||'. I do prefer the pipes since they have less 'aha' things ('and' behaves similar). > For instance: > > ``` > $var = false || true; > var_dump($var === true); // this would be true. > > $var = false or true; > var_dump($var === true); // this would be false, since $var value is false, since it got immediately assigned to $var; 'true' got evaluated but not assigned. > // in order to fix this we'd need to wrap it between parenthesis: > $var = (false or true); > var_dump($var === true); // dumps true. > ``` > >

esh I NEVER intend to use or -- I'll fix this. it never behaves as expected and IMO doesn't add anything. I literally have no idea how to use or meaningfully in PHP!

_test/JQStore_AllTest.php, line 257 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > I'd pass the queue number as an arg, and prob the job count across the queues too. Maybe it's not that important here because this is a test, but still feels pretty rigid to me. >

i can see that, thanks. i don't need the flexibility but I can appreciate how it makes the tests that leverage this setup helper more readable.

_test/JQStore_AllTest.php, line 284 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > .... If you pass the queue count as an arg, and the jobs count across the queues, as in `$this->setupJobsInQueues($q, $queueCount = 3, $jobsPerQueue = 1);`, > > then this assert would read: > > ``` > $totalJobs = $queueCount * $jobsPerQueue; > $this->assertEquals($totalJobs, $w->jobsProcessed()); > ``` > > The main benefit is that if for some reason we later need to assert 4 total jobs, then we'll need to also edit the setupOneJobInThreeQueues() code, and refactor all other tests that make use of this method. > > Also that the '3' in the assert seems kind of magic since it's correlated to a method that happens to insert 3 jobs. > > But I think it mainly depends on how likely these tests will change in future. If it's very unlikely then I would leave it as it is. Thoughts? >

:thumbsup: I will clean that up, thanks.

Comments from Reviewable

apinstein commented 8 years ago

Review status: 5 of 8 files reviewed at latest revision, 6 unresolved discussions.

a discussion (no related file):

Previously, apinstein (Alan Pinstein) wrote… > I'd expect no BC issues; the new feature wouldn't change how existing things are flowing. > > It is always a client-of-jqjobs concern to reason about how queues are organized. This is classic queueing theory and operational management. Tons of academic info about it (I studied it in school, see https://en.wikipedia.org/wiki/Queueing_theory ) > > Should we link to that in the readme as a guide for people to grok how to coordinate the queues? >

I linked to that WIKIPEDIA article.

src/JQJobs/JQManagedJob.php, line 364 [r2] (raw file):

Previously, apinstein (Alan Pinstein) wrote… > esh I NEVER intend to use `or` -- I'll fix this. it never behaves as expected and IMO doesn't add anything. I literally have no idea how to use `or` meaningfully in PHP! >

fixed

_test/JQStore_AllTest.php, line 257 [r2] (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > i can see that, thanks. i don't need the flexibility but I can appreciate how it makes the tests that leverage this setup helper more readable. >

fixed. longer term I could see this being generalized to accept a func to specify the job, too, and then replace all the similar setup funcs at the top of the test script. But I think it might get tediuos there, too. lots to specify. Would probably end up using wrappers of the underlying "heavy duty" one to generate simpler ones (very similar to that react article about wrapping/composition).

_test/JQStore_AllTest.php, line 284 [r2] (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > :thumbsup: I will clean that up, thanks. >

Done.

Comments from Reviewable

leftdevel commented 8 years ago

Reviewed 2 of 3 files at r3, 1 of 1 files at r4. Review status: all files reviewed at latest revision, 5 unresolved discussions.

README.md, line 40 [r4] (raw file):

Additional Utilities:
* JQAutoscaler is a utility that can manage auto-scaling your worker pool.
y JQDelayedJob is a utility class for registering a function or job to be run after the script exits.

Typo? Did you meant to remove the * ?

_src/JQJobs/JQManagedJob.php, line 362 [r2] (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > this is just a PoC -- it was purposefully not doc'd. Sorry if I didn't make that clear. I am just trying to figure out if the main architectural changes make sense -- I only did a small reference implementation to reason about how hard it would be to technically implement the feature. > > I'm more interested in your opinion on where STRING 'a,b' format should be accepted, and where we force it to more rigorous types. >

I think the 'a,b' is confusing and hard to guess this format. About the arch, I like that it can handle multiple stores. I like the way you explained why this is useful like, one queue can handle long standing jobs only, and other queues can handle quick jobs, this would make it a lot easier to monitor and to plot. Stats would be more meaningful by separating the queues (in those examples).

src/JQJobs/JQManagedJob.php, line 364 [r2] (raw file):

Previously, apinstein (Alan Pinstein) wrote… > fixed >

Nice one!

_test/JQStore_AllTest.php, line 257 [r2] (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > fixed. longer term I could see this being generalized to accept a func to specify the job, too, and then replace all the similar setup funcs at the top of the test script. But I think it might get tediuos there, too. lots to specify. Would probably end up using wrappers of the underlying "heavy duty" one to generate simpler ones (very similar to that react article about wrapping/composition). >

Good observation.

_test/JQStore_AllTest.php, line 325 [r4] (raw file):_

        $w->start();

        $this->assertEquals(3, $w->jobsProcessed());

Maybe I'm being too picky, but prob I'd improve on the assert magic number in $this->assertEquals(3, $w->jobsProcessed());.

What about:

$queueNames = ['a', 'b', 'c'];
$jobsPerQueue = 1;
$this->setupNQueuesWithMJobs($q, $queueNames, $jobsPerQueue, ...);
$expectedTotalJobs = count($queueNames) * $jobsPerQueue;
 $this->assertEquals($expectedTotalJobs, $w->jobsProcessed());

Something along those lines. Thoughts?

Comments from Reviewable

leftdevel commented 8 years ago

Reviewed 5 of 5 files at r5. Review status: all files reviewed at latest revision, 3 unresolved discussions.

_src/JQJobs/JQManagedJob.php, line 362 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > I think the 'a,b' is confusing and hard to guess this format. About the arch, I like that it can handle multiple stores. I like the way you explained why this is useful like, one queue can handle long standing jobs only, and other queues can handle quick jobs, this would make it a lot easier to monitor and to plot. Stats would be more meaningful by separating the queues (in those examples). >

Nice refactor!

Comments from Reviewable

apinstein commented 8 years ago

@leftdevel can you check out this PR now? I think it's good-to-go.

 [X] Test JQStore->next(NULL) finds next job available on all queues
 [X] Test JQStore->next('a') finds next job available on queue 'a' only
 [X] Test JQStore->next('a,b') finds next job available on queues 'a' or 'b'

Review status: 3 of 8 files reviewed at latest revision, 2 unresolved discussions.

README.md, line 40 [r4] (raw file):

Previously, leftdevel (Oscar Balladares M.) wrote… > Typo? Did you meant to remove the `*` ? >

no tx, fixed

_src/JQJobs/JQManagedJob.php, line 362 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > Nice refactor! >

OK cool, I have refactored API to use STRING NAME or ARRAY OF NAMES and move the conversion of 'a,b' => ['a','b'] to the worker constructor.

_test/JQStore_AllTest.php, line 257 [r2] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > Good observation. >

FYI I ended up just finishing the refactor. You can look at it now and see if it seems gross due to being more indirected/generalized.

_test/JQStore_AllTest.php, line 325 [r4] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > Maybe I'm being too picky, but prob I'd improve on the assert magic number in `$this->assertEquals(3, $w->jobsProcessed());`. > > What about: > > ``` > $queueNames = ['a', 'b', 'c']; > $jobsPerQueue = 1; > $this->setupNQueuesWithMJobs($q, $queueNames, $jobsPerQueue, ...); > $expectedTotalJobs = count($queueNames) * $jobsPerQueue; > $this->assertEquals($expectedTotalJobs, $w->jobsProcessed()); > ``` > > Something along those lines. Thoughts? > >

fixed

Comments from Reviewable

leftdevel commented 8 years ago

Reviewed 5 of 5 files at r6. Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/JQJobs/JQWorker.php, line 151 [r6] (raw file):

        if (count($names) === 0)
        {
            return JQManagedJob::QUEUE_ANY;

This trick feels a bit out of band to me. All other lines in this method flows pretty well together. Since this a private method, maybe the usage could be:

$this->options['queueName'] = $this->options['queueName'] ? $this->convertCommaSepQueueNameOptionIntoFormatForNext($this->options['queueName']) :  JQManagedJob::QUEUE_ANY;

But at this point maybe it's not necessary to be too picky about it. I'm sure it works as it is.

src/JQJobs/JQStore/Array.php, line 82 [r6] (raw file):

        }

        if (is_string($queueName))

There is no check for NULL values. I think it's specially important to include it since this is a public function. I'm awre you cast NULL to QUEUE_ANY early in the code though. Maybe this method should be private? (unless it's used outside this class).

Comments from Reviewable

apinstein commented 8 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/JQJobs/JQWorker.php, line 151 [r6] (raw file):

Previously, leftdevel (Oscar Balladares M.) wrote… > This trick feels a bit out of band to me. All other lines in this method flows pretty well together. Since this a private method, maybe the usage could be: > > ``` > $this->options['queueName'] = $this->options['queueName'] ? $this->convertCommaSepQueueNameOptionIntoFormatForNext($this->options['queueName']) : JQManagedJob::QUEUE_ANY; > ``` > > But at this point maybe it's not necessary to be too picky about it. I'm sure it works as it is. >

Are you sure you commented on the right line? A bit confused about your point...

Comments from Reviewable

apinstein commented 8 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/JQJobs/JQStore/Array.php, line 82 [r6] (raw file):

Previously, leftdevel (Oscar Balladares M.) wrote… > There is no check for NULL values. I think it's specially important to include it since this is a public function. I'm awre you cast NULL to QUEUE_ANY early in the code though. Maybe this method should be private? (unless it's used outside this class). >

I believe isAnyQueue() looks for NULL or '(ANY)' [const] ... I don't disagree that it's not super-clear how to handle all this as cleanly. I am not thrilled about it but I am not sure what reasonable refactor makes it better...

Comments from Reviewable

leftdevel commented 8 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/JQJobs/JQWorker.php, line 151 [r6] (raw file):

Previously, apinstein (Alan Pinstein) wrote… > Are you sure you commented on the right line? A bit confused about your point... >

Oui. If you remove the special trick for no items code block, then the usage of this method would looks like the snipped I posted ^ in the constructor. You invoke this method only once, in the __construct().

_src/JQJobs/JQStore/Array.php, line 82 [r6] (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > I believe `isAnyQueue()` looks for `NULL` or '(ANY)' [const] ... I don't disagree that it's not super-clear how to handle all this as cleanly. I am not thrilled about it but I am not sure what reasonable refactor makes it better... >

That's true, it's handled in the isAnyQueue() method.

Comments from Reviewable

apinstein commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

_src/JQJobs/JQWorker.php, line 151 [r6] (raw file):_

Previously, leftdevel (Oscar Balladares M.) wrote… > Oui. If you remove the special trick for no items code block, then the usage of this method would looks like the snipped I posted ^ in the constructor. You invoke this method only once, in the __construct(). >

not totally following... sorry. maybe you can show me while pairing?

Comments from Reviewable

leftdevel commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/JQJobs/JQWorker.php, line 151 [r6] (raw file):

Previously, apinstein (Alan Pinstein) wrote… > not totally following... sorry. maybe you can show me while pairing? >

Yes. Prob this doesn't even require this code to change, but I can show you what I mean on a zoom.

Comments from Reviewable

apinstein commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file): UPDATE: This PR was never merged as the client of JQJobs decided it was simpler to manage queue workloads directly rather than multi-plexed.

The code is complete and pretty tested, so if it's ever needed in the future, here it is. I'd give it 95% confidence of being solid. Would need to be monitored for sure to watch for issues.

Comments from Reviewable