search

Webador / SlmQueueBeanstalkd

Beanstalkd adapter for SlmQueue module
Other
10 stars 26 forks source link

Feature/v0.4 update #40

Closed juriansluiman closed 10 years ago

juriansluiman commented 10 years ago

Update to all v0.4 changes. Supersedes #25, #36 and #38 .

bakura10 commented 10 years ago

By the way, I've just realized Pheanstalk has been in v3 now, with namespaces. It may be good time to introduce it now no? https://github.com/pda/pheanstalk/releases

juriansluiman commented 10 years ago

By the way, I've just realized Pheanstalk has been in v3 now, with namespaces. It may be good time to introduce it now no? https://github.com/pda/pheanstalk/releases

Doing that now :)

Please don't merge yet. I have stumbled upon a few nasty issues where I am looking if this is my config, beanstalkd or SlmQueue.

juriansluiman commented 10 years ago

Currently the issue I am finding is described in #41. Do not merge this PR before above issue is resolved. As far as I know, this was not an issue in v0.3.

juriansluiman commented 10 years ago

41 fixed by 662dcfa1ce60dcc557fb4285bc96971f7c53e1b6 :disappointed:

juriansluiman commented 10 years ago

@bakura10 @basz please review. I am not up2date with all latest changes from SlmQueue, so I might have missed something.

A few things I know:

  1. All tests are passing again, if tests are removed, the commit message explains so
  2. My simple test-app runs fine now with several jobs in the queue tested

What I also noticed with the refactoring:

  1. The controllers for the adapters are empty shells now, we better remove them and rely completely on SlmQueue's controller
  2. The whole id and name changes show that the design is still vulnerable to these changes, we better abstract them away. I forgot a replacement and it caused all kind of other weird errors (see #41). We better have this stuff completely in SlmQueue and not in any of the adapters
  3. What do you think of #38? That allows job's to return their own status codes, which might get very handy if you want to check bitwise if something succeeded and also notify with some other statuses
basz commented 10 years ago

all look sane to me re 1. SlmQueueDoctrine Controller still has a recover action re 2. Isn't that the only place (queue - unserializeJob) id is used? What do you propose unserializeJob($data, $id, $metadata = null)? re 3. no opinion

bakura10 commented 10 years ago
  1. Not really. What about options? (see Sqs: https://github.com/juriansluiman/SlmQueueSqs/blob/master/src/SlmQueueSqs/Controller/SqsWorkerController.php)
  2. I agree. My first idea was to make this configurable through options (this is nice because it allows to avoid BC) but at the same time, I think we won't change id and name anymore. Those are marked as reserved. Therefore the simplest idea is to simply have two constants in the JobInterface like ID_PLACEHOLDER, NAME_PLACEHOLDER and use those in the code.
  3. Sounds nice :p. You should carefully check all the code though that nothing relies on specific constants.
juriansluiman commented 10 years ago

@bakura10 @basz OK let's merge this now then. I will tag v0.4 with this merged.

Re 1: agreed, did not know about those Re 2: It is a micro thing, but for long term maintenance perhaps a good idea, let's see if we can make this into a patch release Re 3: This is not required for a v0.4, so let's delay that for another time, but I will prepare PRs for all three adapters.