Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace queue factory with queue providers #222

Merged
merged 39 commits into from
Dec 7, 2024
Merged

Replace queue factory with queue providers #222

merged 39 commits into from
Dec 7, 2024

Conversation

vjik
Copy link
Member

@vjik vjik commented Nov 1, 2024

Q A
Is bugfix?
New feature?
Breaks BC? ✔️

@samdark samdark requested a review from viktorprogger November 1, 2024 14:17
config/params.php Outdated Show resolved Hide resolved
@vjik vjik marked this pull request as ready for review November 7, 2024 07:51
@vjik vjik requested a review from a team November 7, 2024 07:51
@vjik vjik added the status:code review The pull request needs review. label Nov 7, 2024
src/Adapter/StubAdapter.php Outdated Show resolved Hide resolved
src/Cli/StubLoop.php Outdated Show resolved Hide resolved
src/Command/ListenAllCommand.php Outdated Show resolved Hide resolved
src/Command/ListenAllCommand.php Outdated Show resolved Hide resolved
src/QueueFactory.php Show resolved Hide resolved
src/Queue.php Outdated Show resolved Hide resolved

public function get(string $channel): QueueInterface
{
return $this->baseQueue->withChannelName($channel);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not enough just to set a channel name into a queue object. In the old factory we used a default (base) adapter too:

return $this->queue->withChannelName($channel)->withAdapter($this->defaultAdapter->withChannel($channel));

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan fix it another PR. withChannelName in Queue should be enough and Queue should configure own adapter himself.

Copy link
Contributor

@viktorprogger viktorprogger Nov 17, 2024

Choose a reason for hiding this comment

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

Anyway this PR should work as expected before merging it into master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in Queue::withChannelName.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this functionality shouldn't be accessible by default, as it's too easy to make a mistake and run out of resources on a production server by spamming with new open connections. In the QueueFactory this functionality was protected by the enableRuntimeChannelDefinition configuration flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to remove Queue::withChannelName metod in future PR.

config/di.php Outdated
use Yiisoft\Queue\QueueInterface;
use Yiisoft\Queue\Worker\Worker as QueueWorker;
use Yiisoft\Queue\Worker\WorkerInterface;

/* @var array $params */

return [
QueueProviderInterface::class => [
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add definitions for all the providers, so if users want to use one of them, they can do it easily with just setting a definition for the provider interface.

AdapterFactoryQueueProvider::class => [...],
QueueFactoryQueueProvider::class => [...],

// and set the default one
QueueProviderInterface::class => AdapterFactoryQueueProvider::class,

Copy link
Member Author

Choose a reason for hiding this comment

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

Now the configuration is as it was before. I want to improve package configuration in one of the next PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no guarantee those future PRs will be merged into master soon. Those who already use this package should have a convenient configuration now. Let's do it before the PR is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently package in dev status. It's OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a not released package I'm for making a breaking change, but I'm against lack of configuration. I have two arguments for this:

  • Some people already use this package. I'd prefer to make its usage as comfortable as we can. Despite there are breaking changes.
  • If a change needs to have a default configuration, it should have a default configuration from the very beginning. This way we'll avoid decisions, which are good at architectural point, but are hard to be used in real-life projects. The goal is to provide some default configuration, document it and see if it's really comfortable to use in a project. If it's impossible to describe it in simple words - than the decision is likely to be reimagined and simplified for the end user.

src/StubQueue.php Outdated Show resolved Hide resolved
@vjik vjik requested a review from viktorprogger November 11, 2024 11:52
@viktorprogger viktorprogger changed the title Replace queue factory to queue provider Replace queue factory with queue providers Nov 17, 2024
@vjik vjik merged commit 3dd8a7b into master Dec 7, 2024
19 checks passed
@vjik vjik deleted the queue-factory branch December 7, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants