New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Introducing several executor services for IRI #1131

Merged
merged 4 commits into from Nov 12, 2018

Conversation

Projects
None yet
2 participants
@hmoog
Copy link
Contributor

hmoog commented Nov 8, 2018

Description

This PR introduces ExecutorServices that will be used for the background workers of IRI (and in particular for local snapshots and its related classes). The introduced ExecutorServices allow the scheduling of Tasks with automated logging capabilities without using "while(!shuttingDown) { ... }" loops by using the well known ScheduledExecutorService interface.

Once this Feature is merged - another PR will replace the "old" thread handling of the Local Snapshot classes.

Type of change

  • Enhancement (a non-breaking change which adds functionality)

Code Example:

private final static Logger logger = LoggerFactor.getLogger(MilestoneSolidifier.class);

private DedicatedScheduledExecutorService milestoneSolidifier = new DedicatedScheduledExecutorService(
        "Solidification Thread", logger);

// calling this multiple times will only start exactly one background job (ignore additional requests)
public void start() {
    milestoneSolidifier.silentScheduleAtFixedRate(this::solidificationThread, 10, 50, MILLISECONDS);
}

// calling this multiple times will only stop the one instance that is running (if it is running)
public void shutdown() {
    milestoneSolidifier.shutdownNow();
}

public void solidificationThread() {
    System.out.println("I get executed every 50 milliseconds");
}

Resulting Log Output:

[main] INFO  MilestoneSolidifier - Starting [Milestone Solidifier] (starts in 10ms / runs every 50ms) ...
[main] INFO  MilestoneSolidifier - [Milestone Solidifier] Started (execution #1) ...
[Milestone Solidifier] INFO  c.i.i.s.m.MilestoneSolidifier - I get executed every 50 milliseconds
[Milestone Solidifier] INFO  c.i.i.s.m.MilestoneSolidifier - I get executed every 50 milliseconds
[Milestone Solidifier] INFO  c.i.i.s.m.MilestoneSolidifier - I get executed every 50 milliseconds
[main] INFO  MilestoneSolidifier - Stopping [Milestone Solidifier] ...
[main] INFO  MilestoneSolidifier - [Milestone Solidifier] Stopped (after #4 executions) ...

Checklist:

Please delete items that are not relevant.

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 9, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Nice,

I just had a few questions and javadocs nits

Show resolved Hide resolved src/main/java/com/iota/iri/utils/thread/ReportingExecutorService.java
Show resolved Hide resolved src/main/java/com/iota/iri/utils/thread/ReportingExecutorService.java
Show resolved Hide resolved ...main/java/com/iota/iri/utils/thread/BoundedScheduledExecutorService.java Outdated
* @param <ARGUMENT> type of the wrapped command that is passed in ({@link Runnable} vs {@link Callable})
*/
@FunctionalInterface
private interface FutureFactory<RESULT, ARGUMENT> {

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 11, 2018

Member

It is funny, I was so used to only using one letter in type params it never occured to me that we can write a word 😆

Anyhows, 👍 on the clarity

Show resolved Hide resolved ...main/java/com/iota/iri/utils/thread/BoundedScheduledExecutorService.java Outdated
Show resolved Hide resolved ...main/java/com/iota/iri/utils/thread/BoundedScheduledExecutorService.java

this.threadName = threadName;
this.logger = logger;
this.debug = debug;

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 11, 2018

Member

Instead of having a separate debug flag can we maybe use logger.isDebug() throughout the class logic?

This comment has been minimized.

@hmoog

hmoog Nov 12, 2018

Contributor

logger.isDebug() indicates the "logging level" and determines if "debug" messages are shown. This debug flag however addresses sth else. It is a flag that controls if "ALL" callbacks print messages (every start + every stop rather than the first start and the last stop - like a normal Thread).

In case of problems with the Thread it can for example be used to check if a method call terminates in time or if it gets stuck at the first iteration (I used it for sth like that already where a Thread didnt do what it should). In my case the thread was stuck in an endless loop and never terminated, which was easy to tell by just activating this flag).

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 12, 2018

Member

How do we control the debug flag?
If it is by manually changing in the code it is fine for the dev who works on it, but not if we communicate with a user experiencing problems.
I would think this should be able to activate at least with logger.isTrace() so one can see the messages without changing the code.
Would you agree with that?

This comment has been minimized.

@hmoog

hmoog Nov 12, 2018

Contributor

It's only for the developer to be able to add extra log messages while developing a new task which allows him to determine if something obvious doesn't work as planned (like the given example of a task not terminating). This should not be controlled by the user.

Exceptions or problems that might arise within the scope of a user interacting with IRI should always be handled by the specific logic of the task. Enabling a system wide debug flag for "all" "Threads" in IRI and printing additional "start / stop" messages for all Threads that use this ExecutorService would in fact make it even harder to debug problems because the console would be spammed with completely useless messages that are not related to the problem (depending on the chosen intervals - every few milliseconds).

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 12, 2018

Member

This is why I offered to do in trace mode.
I understand your reasoning though and accept it.

@iotaledger iotaledger deleted a comment from codacy-bot Nov 12, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 12, 2018

@iotaledger iotaledger deleted a comment from hmoog Nov 12, 2018

@iotaledger iotaledger deleted a comment from hmoog Nov 12, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Good Job!

@GalRogozinski GalRogozinski merged commit 4dda1f1 into iotaledger:dev Nov 12, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
buildkite/iri-build-jar-prs Build #56 passed and blocked
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hmoog hmoog deleted the iotadevelopment:merge_thread branch Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment