maxbrokman / SafeQueue

Laravel Doctrine friendly queue worker
MIT License
30 stars 28 forks source link

Architecture Question #10

Closed ChrisSchwerdt closed 7 years ago

ChrisSchwerdt commented 7 years ago

I looked into using this package for a project I'm working on, but I wanted to throw out a question about the architecture of your worker. It seems to me that your worker is a little more fragile than it needs to be because it's re-implementing parent methods (seemingly just for unit test purposes).

It seems to me that it would be better to let the parent class do as much as possible. That way any future improvements or bug fixes to the parent worker don't have to be re-implemented in your child worker.

I created a solution based off your work (which is very much appreciated), but tried to keep it more bare-bones. The core logic of my child worker is below (the only methods I override are __construct() and runNextJob()). Do you think this approach is something you would be interested in taking for your implementation?

public function runNextJob($connectionName, $queue, WorkerOptions $options)
{
    try {
        $this->assertClearAndOpenEntityManager();
        $this->assertDatabaseConnected();

        parent::runNextJob($connectionName, $queue, $options);
    } catch (EntityManagerClosedException $e) {
        $this->exceptions->report($e);

        $this->stop();
    } catch (Exception $e) {
        $this->exceptions->report($e);

        $this->sleep($options->sleep);
    } catch (Throwable $e) {
        $this->exceptions->report(new FatalThrowableError($e));

        $this->sleep($options->sleep);
    }
}
maxbrokman commented 7 years ago

Hi @ChrisSchwerdt this is a good idea and I've tried to implement it here: d16f99b, let me know if you have any thoughts.

ChrisSchwerdt commented 7 years ago

I think it looks great! Even better now that Laravel 5.4 created an explicit runJob() method so there's no need to sleep() in the child class. Thanks for taking my comments into consideration. 👍