Refactoryzacja?

0

Czy może tak być czy z tą wielką pętlą while trzeba coś zrobić?
Podzielić te części na osobne prywatne metody? ma to sens?

public final class IssueProducerByStatusAndUpdateDate implements IssueProducer {

    private static final Logger logger = LoggerFactory.getLogger(IssueProducerByStatusAndUpdateDate.class);

    private final MyService service;
    private final String projectName;
    private final IssueStatus status;
    private final LongSupplier timeFun;
    private final LocalDateTime startDateTime;

    private ConcurrentBuffer<Issue> buffer;

    public IssueProducerByStatusAndUpdateDate(MyService service, String projectName,
                                              IssueStatus wantedStatus, LongSupplier timeFun) {
        this(service, projectName, wantedStatus, timeFun, new LocalDateTime());
    }

    public IssueProducerByStatusAndUpdateDate(MyService service, String projectName, IssueStatus status,
                                              LongSupplier timeFun, LocalDateTime startDateTime) {
        this.service = service;
        this.projectName = projectName;
        this.status = status;
        this.timeFun = timeFun;
        this.startDateTime = startDateTime;
    }

    @Override
    public void run() {
        LocalDateTime checkFromTmp = startDateTime;
        while (true) {
            Iterable<Issue> foundIssues = produce(checkFromTmp);
            for (Issue issue : foundIssues) {
                checkFromTmp = (getAndUpdateNewCheckDate(issue, checkFromTmp));
                try {
                    buffer.put(issue);
                } catch (Exception e) {
                    String msg = "Adding Issue with key " + issue.getKey() + " failed";
                    logger.warn(msg, e);
                }
            }
            logger.info("Producer: buffer size:" + buffer.size());
            try {
                Thread.sleep(timeFun.getAsLong());
            } catch (InterruptedException e) {
                Thread.interrupted();
                String msg = "Issue Producer has been interrupted. Restart app.";
                logger.error(msg, e);
                throw new JiraStatusChangerException(msg);
            }
        }
    }

    @Override
    public Iterable<Issue> produce(LocalDateTime checkFromTmp) {
        Iterable<Issue> issues = service.findAllIssues(projectName);
        return StreamSupport.stream(issues.spliterator(), false)
                .filter(i -> i.getStatus().getName().equals(status.name))
                .filter(i -> i.getUpdateDate().toLocalDateTime().isAfter(checkFromTmp))
                .collect(Collectors.toList());
    }

    @Override
    public void setBuffer(ConcurrentBuffer<Issue> buffer) {
        this.buffer = buffer;
    }

    public Set<Issue> getBufferSnapshot() {
        return buffer.getBufferSnapshot();
    }

    private LocalDateTime getAndUpdateNewCheckDate(Issue issue, LocalDateTime oldDate) {
        LocalDateTime issueUpdateDate = issue.getUpdateDate().toLocalDateTime();
        return issueUpdateDate.isAfter(oldDate) ? issueUpdateDate : oldDate;
    }
}
1

Raczej tylko run() bym rozbił na 3-4 mniejsze metody. Reszta jest ok.

0
TomRiddle napisał(a):

Raczej tylko run() bym rozbił na 3-4 mniejsze metody. Reszta jest ok.

a wtedy obsługa wyjątków wewnątrz tych mniejszych metod czy dalej w run?

0
Julian_ napisał(a):
TomRiddle napisał(a):

Raczej tylko run() bym rozbił na 3-4 mniejsze metody. Reszta jest ok.

a wtedy obsługa wyjątków wewnątrz tych mniejszych metod czy dalej w run?

Że InterruptedException? Jak najbliżej sleep().

PS: Wgl czemu masz catch (Exception)?

5

Ja to zamiast tego run() z pętlą nieskończoną zrobiłbym jakiś Timera z scheduler taskiem. Ten while(true) to mnie o zimny pot przyprawia.

0
MrMadMatt napisał(a):

Ja to zamiast tego run() z pętlą nieskończoną zrobiłbym jakiś Timera z scheduler taskiem. Ten while(true) to mnie o zimny pot przyprawia.

na pewno to jest dobre rozwiązanie zważywszy, że sleep (delay) jest losowy i różny co iterację? Musiałbym za każdym razem tworzyć nowego timera, timer w timerze.

2

Nie musisz nowego timera robić. Zobacz na przykład:

public class Main {

    ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();

    Main() {
        scheduleNext();
    }


    void task() {
        System.out.println("elo");
        scheduleNext();
    }

    void scheduleNext() {
        int delay = new Random().nextInt(10);
        System.out.println("Delay: " + delay);
        executor.schedule(this::task, delay, TimeUnit.SECONDS);
    }


    public static void main(String[] args) {
        new Main();
    }
}

Reszta to kwestia przekazania tego ScheduledExecutorService tam gdzie potrzebujesz

0

Ja sie wiecej z Wami naucze niz w tej pracy.

1

@Julian_:
Gdy odpisywałem Ci na wątek miałem w głowie coś takiego:

public final class IssueProducerByStatusAndUpdateDate implements IssueProducer {

    private static final Logger logger = LoggerFactory.getLogger(IssueProducerByStatusAndUpdateDate.class);

    private final MyService service;
    private final String projectName;
    private final IssueStatus status;
    private final LongSupplier timeFun;
    private final LocalDateTime checkFromTmp;
    private final ScheduledExecutorService executorService;

    private ConcurrentBuffer<Issue> buffer;

    //tu gdzies ten pierwszy konstruktor

    // te 6 argumentow zaczyna mi wygladac zle wiec pisalbym pewnie klase
    // IssueProducerByStatusAndUpdateDate.Params aby do konstruktora pchac jeden obiekt
    // ale to tak poza konkursem sugestia
    public IssueProducerByStatusAndUpdateDate(
            MyService service, 
            String projectName, 
            IssueStatus status,
            LongSupplier timeFun, 
            LocalDateTime startDateTime,
            ScheduledExecutorService executorService) {

        this.service = service;
        this.projectName = projectName;
        this.status = status;
        this.timeFun = timeFun;
        this.checkFromTmp = startDateTime;
        this.executorService = executorService;
        initTimerTask();
    }

    private void initTimerTask() {
        executorService.scheduleAtFixedRate(this::task, 0, timeFun.getAsLong(), TimeUnit.MILISECONDS);
    }

    private void task() {
        Iterable<Issue> foundIssues = produce(checkFromTmp);
        for (Issue issue : foundIssues) {
            checkFromTmp = (getAndUpdateNewCheckDate(issue, checkFromTmp));
            try {
                buffer.put(issue);
            } catch (Exception e) {
                String msg = "Adding Issue with key " + issue.getKey() + " failed";
                logger.warn(msg, e);
            }
        }
        logger.info("Producer: buffer size:" + buffer.size());
    }

    // do wywalenia jak ten interfejs idzie w piach
    @Override
    public void run() {
        // doNothing();
    }

    @Override
    public Iterable<Issue> produce(LocalDateTime checkFromTmp) {
        Iterable<Issue> issues = service.findAllIssues(projectName);
        return StreamSupport.stream(issues.spliterator(), false)
                .filter(i -> i.getStatus().getName().equals(status.name))
                .filter(i -> i.getUpdateDate().toLocalDateTime().isAfter(checkFromTmp))
                .collect(Collectors.toList());
    }

    // a to to do wywalenia chyba bo mozna konstruktorem to ustawic
    @Override
    public void setBuffer(ConcurrentBuffer<Issue> buffer) {
        this.buffer = buffer;
    }

    public Set<Issue> getBufferSnapshot() {
        return buffer.getBufferSnapshot();
    }

    private LocalDateTime getAndUpdateNewCheckDate(Issue issue, LocalDateTime oldDate) {
        LocalDateTime issueUpdateDate = issue.getUpdateDate().toLocalDateTime();
        return issueUpdateDate.isAfter(oldDate) ? issueUpdateDate : oldDate;
    }
}

Jako że nie masz przymusu zachowywania / używania interfejsu mógłbyś ten background-work zaimplementować w taki sposób jak wyżej z ewentualnym wywaleniem tego twojego interfejsu. Od razu zaznaczam że pisałem to w notatniku i nie sprawdzałem czy działa, to tylko szkic koncepcji jak ja bym to zrobił. ;)

Edit: Ewentualnie zamiast robić initTask w kostrutorze, wywołać go na obiekcie po stworzeniu np. new IssueProducerByStatusAndUpdateDate(...).initTask() choć bardziej powinno się to nazywać runTask() ale to kwestia szlifu.

0

No dobra próbuję dalej pozbyć się tego while

zastanawiam się jak wywołąć scheduler w schedulerze:

ScheduledExecutorService exec = Executors.newScheduledThreadPool(2);
// ...
ScheduledExecutorService e = Executors.newSingleThreadScheduledExecutor();
exec.scheduleAtFixedRate(() -> {
            e.schedule(consumer, 5000, TimeUnit.MILLISECONDS);
        }, 1000, 1000, TimeUnit.MILLISECONDS);
class Consumer {
// ...
void run() {
    Thread.sleep(60000);
// ...

nie rozumiem, czemu mi ten consumer śmiga co sekundę, exec nie czeka aż wątek z e się wykona... Jak już to powinien śmigać co 5 sek

0

co sekunde zlecasz zrobienie czegoś co 5 sekund.

Co do drugiego to dokumentacja i różnica między scheduleAtFixedRate a scheduleWithFixedDelay ;)

0
danek napisał(a):

co sekunde zlecasz zrobienie czegoś co 5 sekund.

Co do drugiego to dokumentacja i różnica między scheduleAtFixedRate a scheduleWithFixedDelay ;)

scheduleWithFixedDelay też nie czeka tych 60 sek.

0

Czekaj bo chyba nie rozumem. Opisz jak chcesz zeby było i jak jest

0

Chcę żeby co sekundę uruchomił mi wątek z opóźnieniem 5 sek. Jeśli ten wątek się nie skończył to ma poczekać na niego zanim uruchomi nowy.
Ma być 1 wątek naraz odpalony.

0

co sekundę uruchomił mi wątek z opóźnieniem 5 sek

compile error

Co to znaczy? Chcesz żeby za 5 sekund wątki zaczęły się odpalać co sekundę? A no i ten sleep jest trochę słaby, bo mogą się dziać różne ciekawe rzeczy, bo sleep powoduje yield.

0

A na pewno nie ma to być tak jak pisałeś poprzednim razem że:

  1. Wylosuj opóźnianie np. 5 sek.
  2. Odpal wątek z opóźnieniem 5 sek.
  3. Jak wątek przestanie działać, wylosuj ponownie opóźnienie np. 3 sek.
  4. Odpal wątek z opóźnieniem 3 sek.
    I tak dalej?

Bo z tego co piszesz to sposób zachowania / specyfikacja Ci się zmieniła.

0

Dobra, to się nie da tego zrobić executorami, wracam do while.
Tak samo nie da się kulturalnie zrobić żeby executor się kończył, gdy który kolwiek z wątków zwróci wyjątek.
Wracam do thread1.start(); thread1.join()...

1
Julian_ napisał(a):

Dobra, to się nie da tego zrobić executorami, wracam do while.
Tak samo nie da się kulturalnie zrobić żeby executor się kończył, gdy który kolwiek z wątków zwróci wyjątek.
Wracam do thread1.start(); thread1.join()...

Masz, sprawdź sobie czy o to Ci chodziło i nie płacz więcej ;)

package pl.matadini.julianapp;

import java.util.Random;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.MoreExecutors;

import lombok.RequiredArgsConstructor;

@RequiredArgsConstructor
class RandomExecutor implements Runnable {

    private final ListeningScheduledExecutorService executor;

    private void myMethod() {
        System.out.println("Wykonuje zaplanowana metode");
    }

    private void afterThreadDead() {
        int randomSeconds = getRandomSeconds(0,10);
        System.out.println("Planuje taska...\nWylosowany delay sekundowy: " + randomSeconds);
        runExeturor(randomSeconds);
    }

    private int getRandomSeconds(int x, int y) {
        Random r = new Random();
        return r.nextInt(y - x) + x;
    }

    @Override
    public void run() {
        afterThreadDead(); 
    }

    private void runExeturor(int firstRandom) {
        ListenableFuture<?> submit = executor.schedule(this::myMethod, firstRandom, TimeUnit.SECONDS);
        submit.addListener(this::afterThreadDead, MoreExecutors.directExecutor());
    }

}
/**
 * Hello world!
 */
public final class App {
    private App() {
    }

    /**
     * Says hello to the world.
     * @param args The arguments of the program.
     */
    public static void main(String[] args) {
        ListeningScheduledExecutorService listeningDecorator = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1));
        new RandomExecutor(listeningDecorator).run();
    }
}

BTW. jak coś to niech ktoś mnie poprawi

1 użytkowników online, w tym zalogowanych: 0, gości: 1