Skip to content
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

Make the Repeater class use a threads to execute the checks #493

Merged
merged 5 commits into from
Feb 7, 2017

Conversation

grkvlt
Copy link
Member

@grkvlt grkvlt commented Dec 15, 2016

The Future returned by the Executor can be cancelled if it does not return a value after the required timeout.

The Future returned by the Executor can be cancelled if it does
not return a value after the required timeout.
@geomacy
Copy link
Contributor

geomacy commented Dec 16, 2016

hi @grkvlt looks good at an initial glance, but are there tests that could be added to demonstrate the improvement?

@neykov
Copy link
Member

neykov commented Dec 16, 2016

I like the idea, very useful sometimes.

Brooklyn already spins up a huge number of threads and this change will add even more even if not needed (tasks needs cancelling). Do you think each task will benefit from the change? If not, what if we provide a wrapper to Repeater to be used selectively?
Could be a very thin wrapper which passes a wrapped body which is executed in a separate thread. The body executed by the repeater just does future.get(timeout).

@aledsage
Copy link
Contributor

aledsage commented Jan 5, 2017

@grkvlt I agree with @neykov's concern about thread usage. Maybe we could make it configurable, so the default is the existing behaviour (which would use a guava MoreExecutors.directExecutor()), and only a new single-threaded executor if configured to do so.

@grkvlt
Copy link
Member Author

grkvlt commented Jan 11, 2017

@neykov @aledsage @geomacy thanks for the feedback, I'll make this configurable and add some tests

@grkvlt grkvlt force-pushed the enhancement/threaded-repeater branch from 46cb2d7 to a9b945e Compare January 26, 2017 15:45
@grkvlt
Copy link
Member Author

grkvlt commented Jan 26, 2017

@neykov @aledsage I used MoreExecutors.sameThreadExecutor() as the default, but added threaded() and threaded(ExecutorService) methods for the builder to change to either Executors.newSingleThreadExecutor() or whatever the user wants.

Copy link
Member

@neykov neykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, only concern is shutting down the executor if passed from the outside.

public ReferenceWithError<Boolean> runKeepingError() {
Preconditions.checkState(body != null, "repeat() method has not been called to set the body");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to remove the body != null check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been changed to a checkNotNull() call


Time.sleep(delayThisIteration);
} finally {
executor.shutdownNow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't shut down if passed from caller.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!


Future<?> call = executor.submit(body);
try {
call.get(delayThisIteration.toMilliseconds(), TimeUnit.MILLISECONDS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a separate time out is needed here, especially when used with backoff, but happy to go with this.

@grkvlt grkvlt force-pushed the enhancement/threaded-repeater branch 3 times, most recently from f877b39 to 2003b4b Compare February 2, 2017 17:49
@grkvlt grkvlt force-pushed the enhancement/threaded-repeater branch from 2003b4b to d6041d4 Compare February 2, 2017 18:33
*/
public Repeater threaded(ExecutorService executor) {
this.executor = executor;
return this;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit on the pedantic side - can you add shutdown =false here. Can imagine first calling threaded() and passing the builder to another method only to add a custom executor using threaded(myExecutor). Good to merge after that.

@grkvlt
Copy link
Member Author

grkvlt commented Feb 6, 2017

@neykov have a look now, I updated the comments to be more explicit too

@@ -0,0 +1,1072 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this file committed by mistake? LGTM otherwise.

@grkvlt grkvlt force-pushed the enhancement/threaded-repeater branch from 8f65875 to 2a3da72 Compare February 7, 2017 12:43
@neykov
Copy link
Member

neykov commented Feb 7, 2017

Thanks for updates @grkvlt, merging.

@asfgit asfgit merged commit 2a3da72 into apache:master Feb 7, 2017
asfgit pushed a commit that referenced this pull request Feb 7, 2017
Make the Repeater class use a threads to execute the checks

The Future returned by the Executor can be cancelled if it does not return a value after the required timeout.
@grkvlt grkvlt deleted the enhancement/threaded-repeater branch February 9, 2017 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants