Initial review phase

[adlawson]

Comments welcome for review of the initial state of the library. This extends upon the review required in #1.

Things to checkout:

• Adapters
  • [ ] ArrayAdapter
  • [ ] SQSAdaper
• Handlers
  • [ ] AbstractAcknowledgeHandler, including Batch, Eager and Null concretions
• Message
  • [ ] Message (not much to see)
  • [ ] MessageFactory
• [ ] Client

[wpillar]

In this polling example:

// Receive a maximum of 10 messages
$client->receive(function (MessageInterface $message, Closure $done) {
    // Do some work

    // You should always define a break condition (i.e. timeout, expired session, etc)
    if ($breakCondition) $done();
}, null);

What is $done? Is that a callback that I as the user define somewhere? Or is that something internal for you as the vendor to tidy up after yourself or something?

[sjparkinson]

Doc comment mismatch: https://github.com/graze/queue/blob/master/src/AbstractWorker.php#L20-L24

[adlawson]

I've mentioned the $done closure in the paragraph above

The second argument given to your worker is a Closure you should use to stop polling when you're finished working.

Is it a bit ambiguous?

[wpillar]

I think it is. What I'm not clear on is how I define what $done is? Where does it come from? And what should I use it for? I thought BatchAcknowledgeHandler would stop polling after x messages?

[wpillar]

In this example:

$client->send([
    $client->create('123abc'),
    $client->create('456def')
]);

I'm not clear on why I have to call $client->create() myself to send stuff to the queue? If I always have to do that and can never pass anything without calling $client->create() then can't something internally do that for me?

Also, if I pass anything but a string into it, I'll get a fatal error when Message is constructed when casting to a string. We could probably do something more helpful there.

[wpillar]

I always think with blocks of code like the following, that there's so much inline stuff going on that it's hard to read at first, it might just be me:

$failed = array_merge($failed, array_map(function ($result) use ($messages) {
    return $messages[$result['Id']];
}, $results->getPath('Failed') ?: []));

Even just moving the closure out makes it more readable imho:

$map = function ($result) use ($messages) {
    return $messages[$result['Id']];
};

$failed = array_merge($failed, array_map($map, $results->getPath('Failed') ?: []));

This might just be a preference thing and there's arguments about creating variables that are 'use-once' I guess but wanted to see what you thought?

[wpillar]

This another example of something I find hard to read:

$size = 1 === $batches
    ? ($limit % self::BATCHSIZE_RECEIVE)
    : self::BATCHSIZE_RECEIVE;

[wpillar]

Your SqsAdapter implements AdapterInterface that specifies that an Iterator should be returned by dequeue. But SqsAdapter doesn't appear to return anything, is that intentional? It's fine for that method to return null?

[adlawson]

It's all generatored up. https://github.com/graze/queue/blob/master/src/Adapter/SqsAdapter.php#L133-L136

The Generator class implements Iterator. That was my primary reason to return an iterator from the adapters rather than an array.

[wpillar]

Ah didn't spot the generator. My bad. Nice 5.5ness.

[andyworsley]

@adrmcintyre Can you please review as well?

[wpillar]

EagerAcknowledgementHandler is breaking Interface Segregation, might not be a problem, just flagging.

[wpillar]

NullAcknowledgementHandler is breaking it even more.

[wpillar]

That is a bit strange, you've got an abstract that only 1 and a half of its concretions are implementing fully. Might not be a problem, the NullAcknowledgementHandler is an odd one.

[adlawson]

They're internal protected methods. The interface is the __invoke on the abstract. My argument is it doesn't matter how they're implemented in the concretions. If nothing is supposed to happen then that's cool too.

[adlawson]

Gunna close this, unless there are any further comments?

[adrmcintyre]

Whoa! Hold on there, I'm still up in your repo, reviewin' your codez.

On 19 November 2014 10:51, Andrew Lawson notifications@github.com wrote:

Gunna close this, unless there are any further comments?

— Reply to this email directly or view it on GitHub https://github.com/graze/queue/issues/3#issuecomment-63622487.

[adrmcintyre]

Doc typo - under https://github.com/graze/queue#handlers, BatchAcknowledgeHandler should be BatchAcknowledgementHandler.

[adrmcintyre]

OK I'm done.

[adlawson]

Thanks!