feat: add AMQP broker #30
Conversation
basert
force-pushed
the
amqp
branch
13 times, most recently
from
e5ccf5c to
78bf379
Compare
basert
force-pushed
the
amqp
branch
2 times, most recently
from
a0f161a to
2bd04c8
Compare
src/Queue/RetryableError.php
Outdated
There was a problem hiding this comment.
lets move this to a folder called src/Queue/Error/Retryable.php
|
|
||
| public function retry(Queue $queue, int $limit = null): void; | ||
|
|
||
| public function getQueueSize(Queue $queue, bool $failedJobs = false): int; |
There was a problem hiding this comment.
Do we need to call this getQueueSize() or just size()
There was a problem hiding this comment.
size is not specific enough, as this here is in context of the Publisher.
|
|
||
| namespace Utopia\Queue\Broker; | ||
|
|
||
| readonly class Queue |
There was a problem hiding this comment.
The other two adapters here AMQP & Redis both implement Publisher and Consumer interface. Whereas Queue.php looks like an unrelated class in the context of a Broker. Lets move it to src/Queue/Queue.php
|
|
||
| // 1. Declare the exchange and a dead-letter-exchange. | ||
| $channel->exchange_declare($queue->namespace, AMQPExchangeType::TOPIC, durable: true, auto_delete: false); | ||
| $channel->exchange_declare("{$queue->namespace}.failed", AMQPExchangeType::TOPIC, durable: true, auto_delete: false); |
There was a problem hiding this comment.
queue->namespace.failed is just utopia-queue.failed right?
We need a failed queue per queue like utopia-queue.v1-functions.failed etc.
Can you also clarify the different queues we're using here? compared to what we had before?
There was a problem hiding this comment.
This is an exchange, not a queue. queues are subscribed to an exchange with their routing key.
| { | ||
| $queueName = $queue->name; | ||
| if ($failedJobs) { | ||
| $queueName = $queueName . ".failed"; |
There was a problem hiding this comment.
the queue name should also have the namespace prefix as it was previously
There was a problem hiding this comment.
Namespace is done by exchange, not by queue.
christyjacob4
left a comment
christyjacob4
left a comment
There was a problem hiding this comment.
I think we should target this to main and release the next minor version atleast. not a patch
We can't, the current main has all the changes for coroutines and they are not ready for appwrite. |
No description provided.