Implemented basic lock component by wachterjohannes · Pull Request #35 · php-task/php-task

wachterjohannes · 2017-05-12T08:35:57Z

This PR introduces a simple lock mechanism which blocks all the executions for the related handler when the execution runs.
Docs php-task/docs#5

alexander-schranz · 2017-05-15T14:22:05Z

src/Task/Lock/Storage/FileLock.php

+     */
+    private function getFileName($key)
+    {
+        return $this->lockPath . DIRECTORY_SEPARATOR . preg_replace('/[^a-zA-Z]/', '_', $key) . '.lock';


alexander-schranz · 2017-05-15T14:26:21Z

src/Task/Storage/ArrayStorage/ArrayTaskExecutionRepository.php

            function (TaskExecutionInterface $execution) use ($task) {
                return $execution->getTask()->getUuid() === $task->getUuid()
-                    && in_array($execution->getStatus(), [TaskStatus::PLANNED, TaskStatus::RUNNING]);
+                       && in_array($execution->getStatus(), [TaskStatus::PLANNED, TaskStatus::RUNNING]);


check styleci config

wachterjohannes · 2017-05-15T14:54:43Z

@alexander-schranz fixed

danrot · 2017-05-16T09:15:14Z

src/Task/Lock/Exception/LockConflictException.php

+ * Will be thrown when a conflict is detected:
+ *  + Already acquired key was acquired.
+ *  + Not acquired key was released.
+ *  + Not acquired key was refreshed.


Can't you throw more specific exception instead of one for all cases? This way it will be harder to debug as it have to (You can't directly see from the exception if the key was already acquired or not).

danrot · 2017-05-16T09:31:51Z

src/Task/Lock/Storage/FileLock.php

+/**
+ * Save locks in the filesystem.
+ */
+class FileLock implements StorageInterface


FileLockStorage?

danrot · 2017-05-16T09:32:38Z

src/Task/Lock/StorageInterface.php

+/**
+ * Interface for lock storage.
+ */
+interface StorageInterface


LockStorageInterface?

danrot · 2017-05-16T09:34:15Z

src/Task/Runner/TaskRunner.php

+     * @param mixed $result
+     *
+     * @return TaskExecutionInterface
+     */


Methods should be named like verbs.

danrot · 2017-05-16T09:34:34Z

src/Task/Runner/TaskRunner.php

+     * @param \Exception $exception
+     *
+     * @return TaskExecutionInterface
+     */


Methods should be named like verbs.

danrot · 2017-05-16T09:35:17Z

src/Task/Storage/TaskExecutionRepositoryInterface.php

+     * @return TaskExecutionInterface
     */
-    public function findScheduled();
+    public function findNextScheduled(\DateTime $dateTime = null);


of course (: I will add an upgrade. This interface is only used for internal usage (TaskRunner) so it should only be important for developers.

danrot · 2017-05-16T09:36:53Z

src/Task/Runner/TaskRunner.php

-                    new TaskExecutionEvent($execution->getTask(), $execution)
-                );
+        $runTime = new \DateTime();
+


Is performance a concern here? You are multiplying the database requests this way. Can't you still load all of them at once and keep them in memory before executing them?

No here is the performance not so important. More important is that the execution is always clean and sync with the database. as you see the implementation before has loaded the whole execution which has to be executed. But in the loop, it was always refreshed because the task could reset (or the listener in the bundle) doctrine. So I change it to this more explicit implementation.

wachterjohannes · 2017-05-16T10:00:58Z

@danrot fixed

danrot

The rest looks good, but I don't have anything to test, and setting that up is probably quite some work... Can maybe @alexander-schranz test again?

danrot · 2017-05-16T11:41:16Z

src/Task/Lock/Exception/LockAlreadyAcquiredException.php

+ */
+class LockAlreadyAcquiredException extends LockConflictException
+{
+}


Wouldn't you also add a nice error message here?

danrot · 2017-05-16T11:41:35Z

src/Task/Lock/Exception/LockNotAcquiredException.php

+ */
+class LockNotAcquiredException extends LockConflictException
+{
+}


Same as above, wouldn't a message be nice?

wachterjohannes · 2017-05-16T11:48:45Z

@danrot fixed

refactored task-runner

7f4b5ac

wachterjohannes force-pushed the feature/lock branch 2 times, most recently from 0f9f5a6 to 58a326a Compare May 12, 2017 08:38

wachterjohannes mentioned this pull request May 12, 2017

Added locking mechanism php-task/TaskBundle#39

Merged

implemented basic lock component

85e08dc

wachterjohannes force-pushed the feature/lock branch 4 times, most recently from 7ab6ad7 to 11ff21c Compare May 15, 2017 09:46

wachterjohannes mentioned this pull request May 15, 2017

Added locking section php-task/docs#5

Merged

wachterjohannes requested a review from danrot May 15, 2017 11:39

wachterjohannes force-pushed the feature/lock branch from 11ff21c to 55bcb29 Compare May 15, 2017 11:39

alexander-schranz requested changes May 15, 2017

View reviewed changes

removed strategy from lock component

8497a58

wachterjohannes force-pushed the feature/lock branch from 55bcb29 to 8497a58 Compare May 15, 2017 14:41

danrot requested changes May 16, 2017

View reviewed changes

fixed naming

04b9dad

wachterjohannes force-pushed the feature/lock branch from b1b1571 to 9412634 Compare May 16, 2017 10:00

danrot requested changes May 16, 2017

View reviewed changes

added upgrade note

032c2e3

wachterjohannes force-pushed the feature/lock branch from 9412634 to 032c2e3 Compare May 16, 2017 11:48

danrot approved these changes May 16, 2017

View reviewed changes

wachterjohannes force-pushed the feature/lock branch 3 times, most recently from 6e15480 to e3373f0 Compare May 18, 2017 06:35

added log message for skipped execution

aea787f

wachterjohannes force-pushed the feature/lock branch from e3373f0 to aea787f Compare May 18, 2017 06:35

implemented skip for locked executions

2fc6ad5

wachterjohannes force-pushed the feature/lock branch from 4e69e55 to 2fc6ad5 Compare May 18, 2017 06:57

alexander-schranz approved these changes May 18, 2017

View reviewed changes

alexander-schranz merged commit 34748a4 into master May 18, 2017

wachterjohannes deleted the feature/lock branch May 18, 2017 07:54

Conversation

wachterjohannes commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wachterjohannes commented May 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wachterjohannes commented May 16, 2017

Uh oh!

danrot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wachterjohannes commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wachterjohannes commented May 12, 2017 •

edited

Loading