skip to Main Content

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


  1. 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

    public function handle() {
    
        // this just a made-up example base on how you do your loops, 
        // you need to update this query properly base on your table name and table relationship
        DB::table('purchases')
            ->rightJoin('purchase_groups as pg', 'purchases.id', '=', 'pg.purchase_id')
            ->rightJoin('price_lists as pl', 'pg.id', '=', 'pl.purchase_group_id')
            ->where('pl.ordered', true)
            ->whereNotNull('pl.tax')
            ->chunk(100, function (Collection $purchases)  {
    
                $errors = Cache::get('purchases_errors') ?? [];
    
                foreach ($purchases as $purchase) {
                    // your logic here, 
                    // all data from 3 tables would be on purchase var
                    $errors[] = 'new error';
                }
    
                Cache::put('purchases_errors', $errors, now()->endOfDay() );
            });
    }
    
    Login or Signup to reply.
  2. 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 solve n+1 performance problems and clean the code up a bit.

    $purchaseGroupQuery = PurchaseGroup::whereHas('purchase')
        ->whereHas(['pricelist' => function ($query) {
            return $query->where('ordered', true)->whereNotNull('tax');
        })->with(['pricelist' => function ($query) {
            return $query->selectRaw("REPLACE(
                REPLACE('tax', ',', ''), '.', ''
            ")->groupBy('tax');
        });
    

    Now let’s optimize the code a bit further by using ->chunk to free up memory every x iterations (x being 25 at this point).

    $purchaseGroupQuery->chunk(25, function ($chunks) {
        foreach ($chunks as $purchaseGroup) {
            $purchaseGroupTaxes = $purchaseGroup->pricelists->pluck('tax');
    
            // $purchaseGroupTaxes is now an array of unique tax values for the purchaselists within the given purchasegroup.
        }
    });
    

    The optimizations above have:

    1. removed the need for nested n+1 queries by calling additional relationships in a loop
    2. removed the need to check if relationships exist or are empty within the loop
    3. formatted the tax value ahead of time for you
    4. ensured that the values returned are unique to each $purchaseGroupTaxes
    5. freed memory up between chunks to avoid exhausting our runner.
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search