Stoppable behavior #102

zhuravljov · 2017-07-29T16:38:28Z

Related #87.

The code is simple and anyone can create it inside own project.
But maybe need to create abstract class which will include stopping logics, and cache component should be moved into child?

TerraSkye · 2017-07-29T17:09:25Z

This wont work when your dealing with EA filecache and 2 different services,
It requires to that the runner and pusher use the same cache instance.

zhuravljov · 2017-07-29T17:15:52Z

@TerraSkye, you are right, cache must use common storage only.

added documentation

cebe · 2017-07-30T23:04:32Z

src/behaviors/StopBehavior.php

+     * @param string $id of a job
+     * @return bool
+     */
+    protected function setStopping($id)


probably markAsStopped would be a better name.

Yes, it make sense.

cebe · 2017-07-30T23:05:23Z

src/behaviors/StopBehavior.php

+     */
+    public $cache = 'cache';
+    /**
+     * @var bool


documentation is missing, its not really clear to me what this switch is for.

The option allows to turn off status usage that is not supported for AMQP driver.

It should be in the phpdoc.

cebe · 2017-07-30T23:06:38Z

src/behaviors/StopBehavior.php

+    public function events()
+    {
+        return [
+            Queue::EVENT_BEFORE_EXEC => function (ExecEvent $event) {


making this a beforeExec() method instead of a closure could make it easier to extend this class.

cebe · 2017-07-30T23:07:44Z

This wont work when your dealing with EA filecache and 2 different services,
It requires to that the runner and pusher use the same cache instance.

could be added as a note to the phpdoc.

samdark · 2017-08-27T11:50:22Z

How about unit tests?

zhuravljov · 2017-08-27T12:48:08Z

@samdark, tests have been added.

samdark

Code looks good to me.

cebe

except the naming it looks good to me.

cebe · 2017-09-01T08:19:37Z

src/stoppable/Behavior.php

+ * @author Roman Zhuravlev <zhuravljov@gmail.com>
+ * @since 2.0.1
+ */
+class Behavior extends \yii\base\Behavior


imo it should be yii\queue\behaviors\Stoppable instead of yii\queue\stoppable\Behavior, Behavior as a class name does not make sense to me.

It will be used in a configuration by long name only. As well as closure behavior. Readability shouldn't be impaired.

'queue' => [ //... 'as stoppable' => \yii\queue\stoppable\Behavior::class, 'as closure' => \yii\queue\closure\Behavior::class, ],

I think to make separate behaviors namespace isn't good idea.

It will be used in a configuration by long name only

Why do you think that people uses long names in configuration? I almost always use short names with use statements at the top of the file.

@rob006, I'm not sure. But this make sense for config files, which cover many class definitions from different namespaces.

Anyway, you will resolve name conflicts using namespace aliases. DbConnection, RedisConnection etc. So you can add alias:

use yii\queue\stoppable\Behavior as QueueStoppable;

I talk no about name suffix. Every package class is grouped to namespace by its role, no by class type.

This is extremely inconsistent to be honest. We have yii\queue\serializers for serializers, but doing the same for behaviors is wrong? So now we have at least three conventions for naming behaviors:

yii\queue\cli\Verbose,

yii\queue\LogBehavior,

yii\queue\stoppable\Behavior.

We have Interface suffix for interfaces, Event suffix for events, but name for behavior is simply random.

We have yii\queue\serializers for serializers, but doing the same for behaviors is wrong?

Yes, because serializer implements only one role. It is queue\serializers.

Behavior is queue middleware. And behaviors can solve many different functions. queue\stoppable\Behavior allows to stop a job, queue\closure\Behavior serializes closures, queue\LogBehavior writes logs, etc.

So now we have at least three conventions for naming behaviors

I renamed Verbose to VerboseBehavior, and now we have single naming rule:

yii\queue\<Role><Type> yii\queue\LogBehavior: Role = queue\Log, Type = Behavior yii\queue\closure\Behavior: Role = queue\closure\, Type = Behavior yii\queue\stoppable\Behavior: Role = stoppable\, Type = Behavior yii\queue\PushEvent: Role = queue\Push, Type = Event etc.

I renamed Verbose to VerboseBehavior, and now we have single naming rule:

You clearly have two naming conventions:

yii\queue\<Role><Type>

yii\queue\<role>\<Type>

Anyway, I just hate using general names for specific implementations. It kills readability and productivity. I often use "Search by class name" feature in IDE - it is useless if you name all behaviors as "Behavior". You can't just import such class because you probably will get conflicts and Behavior will be just meaningless if you find it in code.

This is one of common ways of naming:

yii\<mode>\Application

yii\<package>\Module

yii\gii\generators\<entity>\Generator

Illuminate\Redis\Connections\Connection

Illuminate\Database\Connection

<vendor>\<package>\Client

I often use "Search by class name" feature in IDE - it is useless if you name all behaviors as "Behavior".

I'm not sure about all IDEs, but PHPStorm is able to search class usage considering namespace.

This is not a pointless discussion.

In certain discussions there is no right or wrong, just different opinions.
In this particular case, the Yii2 staff could enrich the discussion with their opinions.

In my opinion, the best solution would be to maintain the naming pattern used in Yii2.
After all, it is an extension to a larger project and here is no place to innovate.

cc: @cebe @samdark

Conflicts: CHANGELOG.md

SamMousa · 2018-05-23T13:05:09Z

I don't like this approach at all.

There is no guarantee they will actually be stopped (a cache is not persistent reliable storage).
It uses the cache as some kind of quick storage, that is not what it is for.
If the cache is not reachable for whatever reason, all workers will stop working.
Several drivers already implement remove() (file, db, beanstalk, redis).

The exceptions here are amqp and gearman.

In the cases where you use amqp / gearman and you have multiple workers, you will need a shared cache.
If you are on the same host then you could use files storage, but in that case you could just as well use the file queue driver since you're depending on the file system anyway.
If you're on different hosts you'll probably end up using a cache driver like redis or memcache; in which case you could just as well use redis for your queue in the first place.

As OP mentioned, implementing it in your own project is simple. Since this (in my opinion) is hardly ever the best, or even a good, solution I think adding this behavior will result in worse decisions by developers.

#87: Stoppable behavior

f729a4a

zhuravljov requested a review from cebe July 29, 2017 16:38

zhuravljov and others added 2 commits July 30, 2017 00:08

Fixed typo

008a36b

Update StopBehavior.php

62a7cf4

added documentation

cebe reviewed Jul 30, 2017

View reviewed changes

zhuravljov and others added 3 commits July 31, 2017 20:48

Fixed naming

53b1565

Adjusted wording [skip ci]

ffb0db3

Adjusted code style [skip ci]

8cf8c69

samdark added the type:enhancement Enhancement label Aug 15, 2017

samdark added this to the 2.0.1 milestone Aug 15, 2017

zhuravljov added 2 commits August 27, 2017 12:35

Description of checkWaiting option

80ac6b3

Merge branch 'master' into enh87

0dc5b2d

Tests

34f0725

samdark approved these changes Aug 27, 2017

View reviewed changes

cebe requested changes Sep 1, 2017

View reviewed changes

zhuravljov added 2 commits October 15, 2017 18:23

Base layer of stoppable behavior

c710f30

Merge branch 'master' into enh87

c67c1f0

Conflicts: CHANGELOG.md

zhuravljov modified the milestones: 2.0.1, 2.0.2 Nov 3, 2017

zhuravljov modified the milestones: 2.0.2, 2.0.x Dec 15, 2017

samdark removed this from the 2.0.x milestone May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stoppable behavior #102

Stoppable behavior #102

zhuravljov commented Jul 29, 2017

TerraSkye commented Jul 29, 2017

zhuravljov commented Jul 29, 2017 •

edited

cebe Jul 30, 2017

zhuravljov Jul 31, 2017

cebe Jul 30, 2017

zhuravljov Jul 31, 2017

samdark Aug 14, 2017

cebe Jul 30, 2017

cebe commented Jul 30, 2017

samdark commented Aug 27, 2017

zhuravljov commented Aug 27, 2017

samdark left a comment

cebe left a comment

cebe Sep 1, 2017

zhuravljov Oct 15, 2017

rob006 Oct 15, 2017

zhuravljov Oct 15, 2017

zhuravljov Oct 15, 2017

rob006 Oct 15, 2017

zhuravljov Oct 15, 2017 •

edited

rob006 Oct 15, 2017

zhuravljov Oct 15, 2017

thiagotalma Dec 17, 2017

SamMousa commented May 23, 2018

Stoppable behavior #102

Are you sure you want to change the base?

Stoppable behavior #102

Conversation

zhuravljov commented Jul 29, 2017

TerraSkye commented Jul 29, 2017

zhuravljov commented Jul 29, 2017 • edited

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

cebe commented Jul 30, 2017

samdark commented Aug 27, 2017

zhuravljov commented Aug 27, 2017

samdark left a comment

Choose a reason for hiding this comment

cebe left a 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

zhuravljov Oct 15, 2017 • edited

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

SamMousa commented May 23, 2018

zhuravljov commented Jul 29, 2017 •

edited

zhuravljov Oct 15, 2017 •

edited