Skip to content

Conversation

@manoj-malviya
Copy link

@manoj-malviya manoj-malviya commented Sep 10, 2017

Basic implementation of aws SQS. So far implements 2 authentication methods

Directly embedding key/secret in config

'queue' => [
            'class' => \yii\queue\sqs\Queue::class,
            'url' => '<sqs url>',
            'key' => '<key>',
            'secret' => '<secret>',
            'region' => '<region>',
            'commandClass' => console\controllers\QueueController::class
        ],

if key and secret is not preset it will use Default CredensialProvider
see http://docs.aws.amazon.com/aws-sdk-php/v3/guide/guide/credentials.html#credential-profiles.

Please review and provide feedback.

@dmirogin dmirogin requested a review from zhuravljov September 10, 2017 10:22
use yii\queue\cli\Command as CliCommand;

/**
* Manages application gearman-queue.

Choose a reason for hiding this comment

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

return $this->_client;
}

private $_client;

Choose a reason for hiding this comment

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

properties should be declared before methods

}

/**
* Set the visibilty to reserve message

Choose a reason for hiding this comment

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

Please, correct phpdoc of all methods.

*/
protected function getClient()
{
if ($this->key && $this->secret)

Choose a reason for hiding this comment

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

By PSR-2, open bracket should start in the same line.

/**
* @inheritdoc
*/
protected function pushMessage($message, $ttr, $delay, $priority)
Copy link

@dmirogin dmirogin Sep 10, 2017

Choose a reason for hiding this comment

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

This method is used anywhere ?

Copy link
Contributor

Choose a reason for hiding this comment

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

As I know it's used here

$event->id = $this->pushMessage($message, $event->ttr, $event->delay, $event->priority);

Choose a reason for hiding this comment

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

sorry, i didn't notice.

@samdark samdark closed this Sep 10, 2017
@samdark samdark reopened this Sep 10, 2017
$receiptHandle = $payload['ReceiptHandle'];
$this->getClient()->changeMessageVisibility(array(
'QueueUrl' => $this->url,
'ReceiptHandle' => $queue_handle,
Copy link
Contributor

Choose a reason for hiding this comment

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

probably this variable should be $receiptHandle ?

Copy link
Author

Choose a reason for hiding this comment

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

My bad!, Fixing it.

*/
public function listen()
{
$this->run();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method should do the same as in other queue drivers. $queue->listen() should run an infinite loop with timeout (sleep) to use as a daemon, regardless of payload existence.

@elitemaks
Copy link
Contributor

Hi, will be there any progress with this PR?

@ryusoft
Copy link

ryusoft commented Oct 17, 2017

suggest to base64_encode, and base64_decode for the MessageBody & Payload's Body, as SQS will throw error for PhpSerializer string, due to some characters are not allowed.

@manoj-malviya
Copy link
Author

@ryusoft ,
Sure. Can you share some such cases?

@elitemaks, Will review and update here accordingly.

I am going to push this class in one of testing envoirment this week and if everything fine, will move it to production replacing db queue.

@ryusoft
Copy link

ryusoft commented Oct 26, 2017

Example of error messages:

exception 'Aws\Sqs\Exception\SqsException' with message 'Error executing "SendMessage" on "https://sqs.xxx.amazonaws.com/xxxxxxxxx/qqq"; AWS HTTP error: Client error: POST https://sqs.xxx.amazonaws.com/xxxxxxxxx/qqq resulted in a 400 Bad Request response:

SenderI (truncated...)

InvalidMessageContents (client): Invalid binary character '#x0' was found in the message body, the set of allowed characters is #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF]. - SenderInvalidMessageContentsInvalid binary character '#x0' was found in the message body, the set of allowed characters is #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF].4f600d36-9b97-5a2e-9d0a-5f5dc9195956'

'MessageBody' => "$ttr;$message",
]);

if ($model !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

if ($model === null) {
    return false;
}

return $model['MessageId'];

*/
private function release($payload)
{
if (!empty($payload['ReceiptHandle'])) {
Copy link
Member

Choose a reason for hiding this comment

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

if (empty($payload['ReceiptHandle'])) {
    return false;
}

// rest of the code here

@elitemaks
Copy link
Contributor

@manoj-malviya Hi, is there any progress? There are several requests for changes still pending.

@thiagotalma
Copy link
Contributor

It seems that the author is not available.
Would it be better if someone took over this PR?

It would be a good improvement.

@elitemaks
Copy link
Contributor

I'd like to try, but not sure what is the best way to do it. Should I create a new PR or how it's possible to continue with the current?

@samdark
Copy link
Member

samdark commented Dec 21, 2017

Yes. Create new one and merge commits from this one into it.

@samdark
Copy link
Member

samdark commented Dec 22, 2017

Closed in favor of #192

@samdark samdark closed this Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants