[5.4] option to add a queue 'prefix' to all tube names #18860

browner12 · 2017-04-19T03:47:09Z

if you have multiple sites on a server that use the same queue driver,
there is no way for your queue worker to distinguish which jobs belong
to its application, and which jobs belong to other applications. one
way around this is to use custom tube names. however this is difficult
to ensure custom naming, and doesn’t offer a consistent way to set
these.

by adding a queue prefix config option, we have one location to set our
value, which gets automatically applied when a job is added to the
queue, and when it is pulled off the queue.

all tests are passing, but I have not added any additional ones for this yet.

I believe this is fully backwards compatible, and will only take effect if the user adds a config variable.

if you have multiple sites on a server that use the same queue driver, there is no way for your queue worker to distinguish which jobs belong to its application, and which jobs belong to other applications. one way around this is to use custom tube names. however this is difficult to ensure custom naming, and doesn’t offer a consistent way to set these. by adding a queue prefix config option, we have one location to set our value, which gets automatically applied when a job is added to the queue, and when it is pulled off the queue.

jhoff · 2017-04-19T03:54:08Z

This would be really nice to have. You can prefix Cache and Sessions but not Queues.

mateusjatenee · 2017-04-19T06:28:31Z

src/Illuminate/Queue/QueueManager.php

+     */
+    public function getQueuePrefix()
+    {
+        return isset($this->app['config']['queue.prefix']) ? $this->app['config']['queue.prefix'] : null;


Can't we do return $this->app['config']['queue.prefix'] ?? null here? IIRC Laravel now requires PHP >= 7, doesn't it? Or is it only on master?

Only on master. 5.5 will be PHP 7+, this is targeting 5.4

yah, i had originally written ?? and then caught myself when I realize I was on 5.4

return $this->app['config']->get('queue.prefix')

it appears $this->app['config'] returns an array, not an object, resulting in a 'call to member function get() on array' error.

$this->app['config'] must return object. Object of class Illuminate\Config\Repository

I don't believe I have any control over that. I'm using it identically to other places in the file.

$this->app['config']->get(''queue.prefix') and $this->app['config']['queue.prefix'] is have identical behavior. And there no need in isset check, because if this key ('queue.prefix') is not exists then default value (null) will be returned.
Part of the config repository code:

class Repository implements ArrayAccess, ConfigContract { ..... /** * Get the specified configuration value. * * @param string $key * @param mixed $default * @return mixed */ public function get($key, $default = null) { return Arr::get($this->items, $key, $default); } ..... }

I see what's going on. In an actual application it returns the config repository, but in the test it is returning a simple array. I've made @taylorotwell 's suggested change and updated the tests to use a config Repository rather than just the array.

ntzm · 2017-04-19T09:13:48Z

src/Illuminate/Contracts/Queue/Queue.php

+     *
+     * @return string
+     */
+    public function getQueuePrefix();


Changing interfaces is a breaking change. I believe this should target 5.5

@ntzm since the method is in the abstract parent class, all the driver implementations will inherit it, so everything will work fine. do people define implementations that don't extend the abstract class?

on principle I agree with you. some projects seem more lax when adding new methods to the interface. can an owner comment on this?

Is the simple solution to remove it from the contract for 5.4? as I stated no one is required to use the prefix, so we'd be fine without it.

Is the simple solution to remove it from the contract for 5.4? as I stated no one is required to use the prefix, so we'd be fine without it.

Yes it should not be part of the contract in 5.4, and only do so in 5.5. That how it being done before and should remain so.

evsign · 2017-04-19T09:17:07Z

src/Illuminate/Queue/BeanstalkdQueue.php

@@ -148,7 +148,7 @@ public function deleteMessage($queue, $id)
     */
    public function getQueue($queue)
    {
-        return $queue ?: $this->default;
+        return $this->getQueuePrefix().($queue ?: $this->default);


I think there would be better to create method in parent class, like getQueueName($queue), and call it in all driver classes.

if I'm following you correctly, the problem with moving the getQueue method to the parent is each driver has a different implementation. take a look at the SqsQueue file, as it's the weirdest of them all.

I meant a little different. I mean create new method like resolveQueueName($queue) and call it in getQueue.

protected function resolveQueueName($queue) : string { return $this->getQueuePrefix() . ($queue ?? $this->default); }

ok, i think i'm following you now. correct me if I'm wrong. the code

$this->getQueuePrefix().($queue ?: $this->default)

appears in 3 of the queue drivers, so you're suggesting adding a method in the parent to handle this?

I agree this would work. I don't know if I see a huge benefit adding it. Might just be an additional function/layer that adds to the complexity. I'm gonna leave it as is for now, but try a PR if this gets merged.

evsign · 2017-04-19T09:21:10Z

Perhaps, you forgot about the Database queue driver.

browner12 · 2017-04-19T14:20:59Z

@evsign the reason I left it off the database driver is because since the queue is in the database, that gives us our separation between different sites on the same server. your sites aren't sharing databases, so there's no chance of crosstalk.

is there another reason you think it would be beneficial in that driver?

evsign · 2017-04-19T14:51:26Z

@browner12 User may have separated database for queues for all sites. In any way, i think all drivers should behave as much as possible in the same way.

browner12 · 2017-04-19T15:01:10Z

@evsign doesn't the queue driver use a table to store the queues, not a whole separate database?

I guess I'm okay either way here. I'll wait for an owner to comment on this, and then add it if they want it.

evsign · 2017-04-19T15:12:27Z

@browner12 separated database with shared table) In case of redis driver you also can create new connection with different database, or just change database for existed one for every application and use it to avoid collisions or misunderstandings, but prefix do the job too)

tillkruss · 2017-04-19T23:14:41Z

Why not just use a separate SQL and Redis database for your installations?

it was pointed out to me that we can’t change contracts on 5.4, so we’ll set it back to the original here. these 2 methods can be targeted to 5.5, however.

also needed to update the tests to actually use a config repository rather than just an array, otherwise the tests will fail with a ‘call to member function get() on array’ message

laurencei · 2017-04-20T16:37:58Z

@tillkruss - as a side point - would this fix the redis cluster tube issue that was discussed a few weeks ago - since you could prefix the queue name with {tube} or something?

tillkruss · 2017-04-20T16:40:03Z

Yeah, it would make that easier.

laurencei · 2017-04-21T13:32:05Z

My personal opinion is if you are adding a global queue prefix - then it needs to apply to all queue drivers. It seems strange to specifically exclude one. There is no performance overhead to add it to database - so why not do it?

Plus - it is confusing, because your PR on laravel/laravel does not explain to users that it only potentially applies to some queue drivers.

So I like the concept of this PR - but I think it needs to be all or none...

browner12 · 2017-04-21T13:56:40Z

it was added to the database driver a couple days ago.

laurencei · 2017-04-21T14:01:31Z

Ok cool. Sorry - I was just going off the comments discussion.

👍

jaketoolson · 2017-04-21T17:42:33Z

How is it @GrahamCampbell didn't close this, give it a thumbs down, and never explain his position? It's a great PR/idea, so I expected it not to be approved.

ntzm · 2017-04-22T08:10:18Z

Kind of rude for no reason @jaketoolson, he's been much better recently

it-can · 2017-04-28T08:30:04Z

@browner12 I was using this in the latest laravel 5.4.20 version with beanstalkd, it seems to add the prefix twice... When listing the tubes in beanstalk I see something weird...

without prefix:

portal

with prefix "server1"

server1server1portal

I will add a seperate issue for this

browner12 · 2017-04-28T14:02:23Z

I'm out of town this weekend but I'll try and take a look when I get back. Andrew Brown

…

On Apr 28, 2017, at 3:30 AM, M. Vugteveen ***@***.***> wrote: I was using this in the latest laravel version with beanstalkd, it seems to append the prefix twice... I cannot reproduce this. When listing the tubes in beanstalk I see something weird... without prefix: portal with prefix "server1" server1server1portal — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

it-can · 2017-04-28T14:22:53Z

Its already fixed in #18982

browner12 · 2017-04-28T14:31:12Z

Great. Thanks! Andrew Brown

…

On Apr 28, 2017, at 9:23 AM, M. Vugteveen ***@***.***> wrote: Its already fixed in #18982 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

taylorotwell · 2017-04-28T15:08:46Z

This entire feature needs to be reverted.

taylorotwell · 2017-04-28T15:09:05Z

It's still breaking random things, and I would probably implement it entirely differently. We have no tests for it either.

browner12 · 2017-04-28T15:13:26Z

sorry guys 🤕 . i'll try and look into this deeper this week.

GrahamCampbell · 2017-04-28T20:35:08Z

For the record, I'm 👎 on this feature, even if it did work. Just adds unnecessary complexity for something that isn't really a problem.

laurencei · 2017-04-28T20:36:46Z

Just to keep this thread as a source in case people come across it later - this was another thing that broke: #18985

browner12 · 2017-04-28T20:37:59Z

thanks man, it'll be good to aggregate them here, because there are a lot of things this touches that I wasn't aware of originally.

franzliedke · 2017-04-28T20:47:13Z

src/Illuminate/Queue/Queue.php

+     * @param  string  $prefix
+     * @return $this
+     */
+    public function setQueuePrefix($prefix = null)


@browner12 Why are these not called setPrefix and getPrefix? After all, we're already in a Queue class here...

I think there was a method on a parent or child class named similarly, so someone suggested it to help reduce ambiguity.

browner12 · 2017-04-28T20:50:53Z

@GrahamCampbell can you explain how you would handle this problem then?

projects should be 100% oblivious to every other project that happens to live on the same server as them. with beanstalk there is no way to ensure that different sites use different queue names.

I really wish beanstalk had a concept similar to a 'shard' like elasticsearch, or some other way to keep them separate.

while this technically doesn't make them 100% oblivious to other projects, it (will) provide a consistent entry point, rather that renaming every single queue everywhere

GrahamCampbell · 2017-04-28T21:20:27Z

projects should be 100% oblivious to every other project that happens to live on the same server as them. with beanstalk there is no way to ensure that different sites use different queue names.

When using something like redis, you can just give each website it's own database. In fact, redis gives you 8 separate ones by default. You can add them all in the laravel config.

GrahamCampbell · 2017-04-28T21:20:51Z

I would imagine beanstalk has a similar concept?

browner12 · 2017-04-29T00:05:43Z

If you find one I'd love to see it. I'll do some digging but I didn't find one before. Andrew Brown

…

On Apr 28, 2017, at 4:20 PM, Graham Campbell ***@***.***> wrote: I would imagine beanstalk has a similar concept? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jhoff · 2017-04-30T03:11:10Z

The "Redis has 8/16 databases that you can use for each Laravel instance" is not a good argument for not having this feature. Sessions, cache and queues can all use redis, but queue is the only thing that can't be prefixed. Of the three, I would argue that queues gain the most benefit from leveraging redis as well. If someone has multiple low volume instances of Laravel that all use redis then they have to keep track of all of these database ids so each app can be isolated.

Point being that with as flexible and portable as Laravel is across the board with the drivers offered for nearly every aspect of the framework, we shouldn't be shooting ideas and features down, just because there's another way to do it. If people wanna drive multiple application queues from a single redis database, let them do it. It's like saying that Postgres isn't gonna be supported because you can already use Mysql.

That said, it's too bad that this implementation didn't work. We have a real need for it. I look forward to v2 of it coming out with full test coverage and all the kinks worked out. Maybe it's better suited for the next major / minor version so people can kick the tires on it a bit first.

browner12 · 2017-05-01T14:34:50Z

I've read through the beanstalkd documentation, and it appears they have the concept of a 'connection', and I'm hoping that's what we're looking for. i've emailed the author of beanstalkd to see if he can shed a little more light on it, and see if it's appropriate for our use.

this PR goes along with laravel/framework#18860

browner12 added 2 commits April 18, 2017 22:45

style fix

116cc95

mateusjatenee reviewed Apr 19, 2017

View reviewed changes

ntzm reviewed Apr 19, 2017

View reviewed changes

evsign reviewed Apr 19, 2017

View reviewed changes

add prefix to database queue driver

17327b7

tillkruss changed the title ~~option to add a queue 'prefix' to all tube names~~ [5.4] option to add a queue 'prefix' to all tube names Apr 19, 2017

browner12 added 2 commits April 19, 2017 19:44

revert new methods on contract

1c40e8f

it was pointed out to me that we can’t change contracts on 5.4, so we’ll set it back to the original here. these 2 methods can be targeted to 5.5, however.

use get method on config repository to access value

6640139

also needed to update the tests to actually use a config repository rather than just an array, otherwise the tests will fail with a ‘call to member function get() on array’ message

browner12 mentioned this pull request Apr 20, 2017

add queue prefix value laravel/laravel#4235

Merged

taylorotwell merged commit 6640139 into laravel:5.4 Apr 21, 2017

browner12 deleted the queue-prefix branch April 21, 2017 16:43

it-can mentioned this pull request Apr 28, 2017

[5.4] Queue prefix added twice with beanstalk #18978

Closed

laurencei mentioned this pull request Apr 28, 2017

[5.4] Fix Queue Prefix bug #18982

Merged

GrahamCampbell mentioned this pull request Apr 28, 2017

Revert Queue Prefix laravel/laravel#4247

Merged

franzliedke reviewed Apr 28, 2017

View reviewed changes

evalesoftware pushed a commit to evalesoftware/eVALE that referenced this pull request Nov 23, 2017

add queue prefix value

5942b21

this PR goes along with laravel/framework#18860

[5.4] option to add a queue 'prefix' to all tube names #18860

[5.4] option to add a queue 'prefix' to all tube names #18860

Conversation

browner12 commented Apr 19, 2017

jhoff commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evsign Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evsign commented Apr 19, 2017 • edited Loading

browner12 commented Apr 19, 2017

evsign commented Apr 19, 2017

browner12 commented Apr 19, 2017

evsign commented Apr 19, 2017

tillkruss commented Apr 19, 2017

laurencei commented Apr 20, 2017 • edited Loading

tillkruss commented Apr 20, 2017

laurencei commented Apr 21, 2017

browner12 commented Apr 21, 2017

laurencei commented Apr 21, 2017

jaketoolson commented Apr 21, 2017 • edited Loading

ntzm commented Apr 22, 2017

it-can commented Apr 28, 2017 • edited Loading

browner12 commented Apr 28, 2017 via email

it-can commented Apr 28, 2017

browner12 commented Apr 28, 2017 via email

taylorotwell commented Apr 28, 2017

taylorotwell commented Apr 28, 2017

browner12 commented Apr 28, 2017

GrahamCampbell commented Apr 28, 2017

laurencei commented Apr 28, 2017

browner12 commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

browner12 commented Apr 28, 2017

GrahamCampbell commented Apr 28, 2017

GrahamCampbell commented Apr 28, 2017

browner12 commented Apr 29, 2017 via email

jhoff commented Apr 30, 2017 • edited Loading

browner12 commented May 1, 2017

evsign Apr 20, 2017 •

edited

Loading

evsign commented Apr 19, 2017 •

edited

Loading

laurencei commented Apr 20, 2017 •

edited

Loading

jaketoolson commented Apr 21, 2017 •

edited

Loading

it-can commented Apr 28, 2017 •

edited

Loading

jhoff commented Apr 30, 2017 •

edited

Loading