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

issue #61 #69

Closed
wants to merge 7 commits into from
Closed

issue #61 #69

wants to merge 7 commits into from

Conversation

tmjee
Copy link

@tmjee tmjee commented Oct 3, 2015

Fix issue #61 Get rid of AspectJ

tmjee and others added 2 commits October 3, 2015 20:20
@tmjee tmjee mentioned this pull request Oct 3, 2015
@dmarkov
Copy link

dmarkov commented Oct 5, 2015

@tmjee I will find a reviewer for your pull requests shortly, thanks for contribution!

@dmarkov
Copy link

dmarkov commented Oct 5, 2015

@longtimeago could you please review this pull request

} .call();
};
final ExecutorService executorService = Executors.newFixedThreadPool(5);
for (int count = zero; count < fifty; count = count + 1) {

Choose a reason for hiding this comment

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

@tmjee It's better to create all Callable instances ant then submit them via ExecutorService#invokeAll()

Copy link
Author

Choose a reason for hiding this comment

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

@longtimeago if we do

for (...) {
    callables.add(
         new Callable<..>(){...};
     )
}
executor.invokeAll(callables);

we'll get checkstyle warnings AvoidInstantiating objectsInLoops

Choose a reason for hiding this comment

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

@tmjee well ... Agreed ... as our Callable has no state we can reuse it many time

@longtimeago
Copy link

@tmjee please, see several remarks above

@tmjee
Copy link
Author

tmjee commented Oct 7, 2015

@longtimeago done.

executorService.submit(callable);
}
executorService.shutdown();
executorService.awaitTermination(tentimes, TimeUnit.SECONDS);

Choose a reason for hiding this comment

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

@tmjee Hereinafter, awaitTermination returns true if all tasks are finished successfully in given time.
As I said in on of my comments, we should assert that awaitTermination returns true and fail overwise.

MatcherAssert.assertThat(executorService.awaitTermination(tentimes, TimeUnit.SECONDS), Matchers.is(true));

@longtimeago
Copy link

@tmjee And one more round, we are very close to finish :)

@tmjee
Copy link
Author

tmjee commented Oct 8, 2015

@longtimeago done.

for (int count = zero; count < ten; count = count + 1) {
executorService.submit(callable);
}
executorService.awaitTermination(ten, TimeUnit.SECONDS);

Choose a reason for hiding this comment

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

@tmjee what about proper shutdown sequence here?

for (int count = zero; count < fifty; count = count + 1) {
executorService.submit(runnable);
}
executorService.awaitTermination(ten, TimeUnit.SECONDS);

Choose a reason for hiding this comment

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

@tmjee and here

@longtimeago
Copy link

@tmjee please, be more attentive. Several more remarks

@tmjee
Copy link
Author

tmjee commented Oct 8, 2015

@longtimeago done

@longtimeago
Copy link

@tmjee and one more

@longtimeago
Copy link

@tmjee now it's good. Thank you!

@longtimeago
Copy link

@rultor merge

@rultor
Copy link
Contributor

rultor commented Oct 8, 2015

@rultor merge

@longtimeago Thanks for your request. @yegor256 Please confirm this.

@yegor256
Copy link
Member

yegor256 commented Oct 9, 2015

@rultor try to merge

@rultor
Copy link
Contributor

rultor commented Oct 9, 2015

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor
Copy link
Contributor

rultor commented Oct 9, 2015

@rultor try to merge

@tmjee @yegor256 Head repository is gone, can't merge from it

@yegor256
Copy link
Member

yegor256 commented Oct 9, 2015

@tmjee what happened with the fork? you deleted it? we can't merge

@tmjee
Copy link
Author

tmjee commented Oct 10, 2015

@yegor256 lost that branch. Created new one #71 or you can get another person to work on this. Thx

@longtimeago
Copy link

@tmjee not a problem. Please, close this one.

@tmjee tmjee closed this Oct 10, 2015
@dmarkov
Copy link

dmarkov commented Oct 15, 2015

@longtimeago Thanks for your contribution, 22 mins was added to your account, payment ID is 67396931, 191 hours and 18 mins spent total. extra minutes for review comments (c=7). +22 to your rating, your total score is +2534

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

Successfully merging this pull request may close these issues.

6 participants