seomoz / qless

Queue / Pipeline Management
MIT License
294 stars 76 forks source link

Allow users to supply a logging formatter #205

Closed tpickett66 closed 9 years ago

tpickett66 commented 9 years ago

In order to ease aggregation and analysis of logging information a consistent logging format is necessary, this commit allows users to pass in a logging formatter of their choice to unify Qless' logging output with the output of their application.

dlecocq commented 9 years ago

Philosophically on board, subject to the build passing.

tpickett66 commented 9 years ago

The failure is only being encountered on one version of ruby and should be happening to all or none. My suspicion is that this is a transient failure but I can't repeat the build on travis since I'm not a project owner.

1) Qless::Workers::SerialWorker can start a worker and then shut it down
     Failure/Error: redis.brpop(key, timeout: 1).should eq([key.to_s, word])

       expected: ["worker_integration_job", "foo"]
            got: ["worker_integration_job", "bar"]

       (compared using ==)
     # ./spec/integration/workers/serial_spec.rb:64:in 'block (4 levels) in <module:Qless>'
     # ./spec/integration/workers/serial_spec.rb:63:in 'each'
     # ./spec/integration/workers/serial_spec.rb:63:in 'block (3 levels) in <module:Qless>'
     # ./lib/qless/test_helpers/worker_helpers.rb:29:in 'block in run_jobs'

After looking through the spec I'm guessing that we're seeing either a race condition in the spec or a minor bug in the time based prioritization code. I see at least 3 ways of making this spec more robust:

  1. Add an explicit priority to each job using the index of the word as the priority (e.g. foo: 0, bar: 1, howdy: 2)
  2. Add a slight delay in the enqueue loop between each job to ensure a sufficiently different timestamp
  3. Allow the queue to be drained completely before checking for any results then check for the entire result ignoring order.

My preference would be to go with option 3, what do you think?

dlecocq commented 9 years ago

Awesome -- I'm glad this one has gotten sorted! LGTM, but @benkirzhner or someone more embedded in the Ruby community might want to give this a once over.