I’ve got questiong about cron and job. My cron is running too long cause there is too much data. How is possible to separate the code to two smaller tasks. Maybe first task can proccess data, save them somewhere and next task will do compare bewteen them ? This task shoul do that it will log just pricelists which has different tax vat inside one purchase. This is my first experience witch chaining and docs didnt help me much. I will be glad for any advice 🙂
This is my job
<?php
namespace AppJobsPurchase;
use AppModelsPurchase;
use AppJobsNodesSendMail;
use IlluminateBusQueueable;
use IlluminateSupportFacadesLog;
use IlluminateQueueInteractsWithQueue;
use IlluminateContractsQueueShouldQueue;
use IlluminateFoundationBusDispatchable;
class VatValidation implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable;
/**
* Create a new job instance.
*
* @return void
*/
public function __construct()
{
}
/**
* Execute the job.
*
* @return void
*/
public function handle()
{
$purchases = Purchase::get();
$errors = [];
foreach ($purchases as $purchase) {
$purchaseGroups = $purchase->purchasegroups()->get();
if (!$purchaseGroups) // Purchase has no purchase groups
continue;
$purchaseGroupsTaxes = [];
// cycle thru pricelists
foreach ($purchaseGroups as $purchaseGroup) {
$pricelists = $purchaseGroup->pricelists()->where('ordered', true)->get();
if (!$pricelists)
continue;
$purchaseGroupPricelistsTaxes = [];
foreach ($pricelists as $pricelist) {
if (empty($pricelist->tax))
continue;
$taxWithoutDotComma = str_replace(['.', ','], '', $pricelist->tax);
array_push($purchaseGroupPricelistsTaxes, $taxWithoutDotComma);
if (count(array_unique($purchaseGroupPricelistsTaxes, SORT_REGULAR)) > 1)
// Log::info("Different taxes found for", ["pgroup" => $purchaseGroup, "pricelists" => $pricelists, "taxes" => $purchaseGroupPricelistsTaxes]);
array_push($errors, "Different taxes found for", ["pgroup" => $purchaseGroup, "pricelists" => $pricelists, "taxes" => $purchaseGroupPricelistsTaxes]);
else
array_push($purchaseGroupsTaxes, $taxWithoutDotComma); // Taxes are same we need just one value
}
}
if (count(array_unique($purchaseGroupsTaxes, SORT_REGULAR)) > 1)
Log::info("Different taxes in purchasegroups found for", ["purchase" => $purchase->name, "taxes" => $purchaseGroupsTaxes]);
array_push($errors, "Different taxes in purchasegroups found for", ["purchase" => $purchase->name, "taxes" => $purchaseGroupsTaxes]);
}
}
}
And this is cron
<?php
namespace AppJobsCrons;
use Throwable;
use AppModelsPurchase;
use AppJobsNodesSendMail;
use AppModelsPurchasegroup;
use IlluminateBusQueueable;
use AppJobsTraitscronTrait;
use IlluminateSupportFacadesLog;
use AppJobsPurchaseVatValidation;
use IlluminateQueueSerializesModels;
use IlluminateQueueInteractsWithQueue;
use IlluminateContractsQueueShouldQueue;
use IlluminateFoundationBusDispatchable;
use IlluminateContractsQueueShouldBeUnique;
class purchaseVatValidationCron implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, cronTrait;
private $cronID;
/**
* Create a new job instance.
*
* @return void
*/
public function __construct(int $cronID = null,)
{
$this->cronID = $cronID;
$this->queue = 'longRunning';
}
/**
* Execute the job.
*
* @return void
*/
public function handle()
{
VatValidation::dispatch()->onQueue('longRunning');
}
public function failed(Throwable $exception)
{
$this->saveLog($this->cronID, $exception->getMessage(), 'error');
Log::error($exception->getMessage(), ["VatValidationCron error"]);
}
}
I am expecting that somebody more experienced will give me some advice how to solve the problem.
2
Answers
Instead of splitting your job, you should optimize it properly and make sure it uses as little memory as possible, I can’t imagine how much memory your job would eat with all those loops that calls
get
via Eloquent.You can use DB Facades Query with right join (based on how you filter/skip them on your loop), Im not sure about your actual table structure and relationship, but you can do a query like below. that chunks the result, its also better to put the errors in cache so you can easily check it anytime and from anywhere in your application
It seems to me you just need distinct tax values per
PurchaseGroup
without real need for any other data, so let’s simplify that. This will solven+1
performance problems and clean the code up a bit.Now let’s optimize the code a bit further by using
->chunk
to free up memory everyx
iterations (x being 25 at this point).The optimizations above have:
n+1
queries by calling additional relationships in a looptax
value ahead of time for youunique
to each$purchaseGroupTaxes