Gfrktyl

I am in the middle of developing a Spring Boot application, which has a service acting like a bot.

advertisementService.update(Advertisement ad) is a method which parses some HTML. It is important that it cannot execute periodically. For now it is pseudo random, but in the future there will be some logic returning the time depending on information read in previous parsing.

Immediately after the start of the app, it invokes the method scheduleAllChecks().
If a new Advertisement comes up, then it calls scheduleCheck(0, ad);

I am wondering if the solution (I mean the whole service class) is proper and if it can be beter?

package com.gadomski.service;



import com.gadomski.model.Advertisement;

import org.slf4j.Logger;

import org.slf4j.LoggerFactory;

import org.springframework.beans.factory.annotation.Autowired;

import org.springframework.stereotype.Service;



import java.util.Random;

import java.util.concurrent.Executors;

import java.util.concurrent.ScheduledExecutorService;

import java.util.concurrent.ScheduledFuture;

import java.util.concurrent.TimeUnit;



/**

 * Created by Krzysiek on 21.08.2016.

 */



@Service

public class CheckerService {

    private final Logger log = LoggerFactory.getLogger(this.getClass());

    private final int CHECKER_THREADS = 100;

    @Autowired

    AdvertisementService advertisementService;

    private ScheduledExecutorService scheduledExecutorService;



    CheckerService() {

        this.scheduledExecutorService = Executors.newScheduledThreadPool(CHECKER_THREADS);

    }



    public void scheduleCheck(int delaySeconds, Advertisement advertisement) {

        if (advertisement.isActive()) {

            ScheduledFuture<?> check = this.scheduledExecutorService.schedule(

                    () -> {

                        scheduleCheck(getDelayInSeconds(), advertisement);

                        advertisementService.updateAdvertisement(advertisement);

                        log.info("Scheaduled next check for " + advertisement.getUrl() + " for user with id: " + advertisement.getUserId());

                    },

                    delaySeconds, TimeUnit.SECONDS);

        }

    }



    private int getDelayInSeconds() {

        Random rand = new Random();

        return (8 + rand.nextInt(10)) * 60 + rand.nextInt(60);

    }



    public void scheduleAllChecks() {

        Random random = new Random();

        advertisementService.getAllActvieAdvertisements().forEach(a -> scheduleCheck(random.nextInt(3 * 60), a));

    }

}

It appears that you are using Random across multiple threads. I guess you plan to replace it in the future, but still, according to the documentation:

Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs.

You should probably rewrite this to use java.util.concurrent.ThreadLocalRandom. As the docs say:

When applicable, use of ThreadLocalRandom rather than shared Random objects in concurrent programs will typically encounter much less overhead and contention.