Webador / SlmQueue

Laminas / Mezzio module that integrates with various queue management systems.
Other
137 stars 56 forks source link

New events causes confusion #141

Closed jeremyquinton closed 10 years ago

jeremyquinton commented 10 years ago

From version 0.3.* to 0.4.0 some events were removed. A previous developer bound to the events so he could log when an event went on the queue and when it was popped off the queue and also when a worker stops and starts.

I had to upgrade slmQueue to 0.4.0 for a new feature but Im not sure which events to now listen for. If you could let me know I can do a PR for the documentation. This is a BC break that will affect other developers that were using those events.

git diff 50a9ae4c2fd98ca9873ea72ea11f054caab1ac79 40b7e0a4f4c3616b881dd6a232038aa40ff8cd3d src/SlmQueue/Worker/WorkerEvent.php
diff --git a/src/SlmQueue/Worker/WorkerEvent.php b/src/SlmQueue/Worker/WorkerEvent.php
index 5a165a6..27976db 100644
--- a/src/SlmQueue/Worker/WorkerEvent.php
+++ b/src/SlmQueue/Worker/WorkerEvent.php
@@ -12,12 +12,34 @@ use Zend\EventManager\Event;
 class WorkerEvent extends Event
 {
     /**
-     * Various events you can listen to
+     * Various events you can subscribe to
      */
-    const EVENT_PROCESS_QUEUE_PRE  = 'processQueue.pre';
-    const EVENT_PROCESS_QUEUE_POST = 'processQueue.post';
-    const EVENT_PROCESS_JOB_PRE    = 'processJob.pre';
-    const EVENT_PROCESS_JOB_POST   = 'processJob.post';
+    const EVENT_BOOTSTRAP        = 'bootstrap';
+    const EVENT_FINISH           = 'finish';
+    const EVENT_PROCESS_QUEUE    = 'process.queue';
+    const EVENT_PROCESS_JOB      = 'process.job';
+    const EVENT_PROCESS_IDLE     = 'process.idle';
+    const EVENT_PROCESS_STATE    = 'process.state';
juriansluiman commented 10 years ago

:+1: on better docs. I was already thinking of the docs while v0.4 was tagged, but it never made it...

What happens now:

  1. A trigger EVENT_PROCESS_QUEUE triggers the ProcessQueue strategy.
  2. This strategies pops a job from the queue. When no job is returned (e.g. the reserve call times out) an EVENT_PROCESS_IDLE is triggered.
  3. When a job is found, the EVENT_PROCESS_JOB is triggered whereafter a default attached listener calls the processJob() on the worker.

So you have these cases:

  1. If you used EVENT_PROCESS_QUEUE_PRE, now use EVENT_PROCESS_QUEUE with a high priority (>1)
  2. If you used EVENT_PROCESS_QUEUE_POST, now use EVENT_PROCESS_QUEUE with a negative priority (<1, preferably <0)
  3. If you used EVENT_PROCESS_JOB_PRE now use EVENT_PROCESS_JOB with a high priority (>1)
  4. If you used EVENT_PROCESS_JOB_POST, now use EVENT_PROCESS_JOB with a negative priority (<1, preferably <0)

With v0.4 the docs on events have been rewritten, but I haven't even read it yet. If you have any improvements, I would be really happy if you could write those down in a PR.

basz commented 10 years ago
  1. Should be EVENT_BOOTSTRAP
  2. Should be EVENT_FINISH
  3. correct
  4. correct

it's a still bit awkward a.t.m. event naming and the flow of events. I would hope we can get a solid design for that anytime soon. But that needs some collaboration. The docs on event should describe the current behavior pretty ok.

EVENT_PROCESS_QUEUE could be considered for internal use. Listened to by the ProcessQueueStrategy, which triggers EVENT_PROCESS_IDLE and EVENT_PROCESS_JOB.

jeremyquinton commented 10 years ago

Thanks very much for the reply. Your response has cleared things up for me. I think what you have for the event system here is really good and I probably could have done a better job at reading the docs. I read over the docs twice and dont think I can improve them much they are pretty good.

eddiejaoude commented 9 years ago

Just to clarify,

For point 2

If you used EVENT_PROCESS_QUEUE_POST, now use EVENT_PROCESS_QUEUE with a negative priority (<1, preferably <0)

Should be EVENT_FINISH

Using EVENT_FINISH, do I still need to use a negative priority?

basz commented 9 years ago

nope that is not required. Same for EVENT_BOOTSTRAP by the way.

eddiejaoude commented 9 years ago

Thanks :+1: