-
Notifications
You must be signed in to change notification settings - Fork 306
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
PAYARA-4118 Speed up PayaraExecutorService Shutdown #4246
PAYARA-4118 Speed up PayaraExecutorService Shutdown #4246
Conversation
…aExecutorService in GFFileHandler" This reverts commit f60de6a. Reverted as this causes stopping payara to take 5 seconds longer
Jenkins test please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just little formatting things here and there and a couple spelling mistakes in comments - all cosmetics otherwise good
} else if (event.is(EventTypes.PREPARE_SHUTDOWN)) { | ||
stopThreadPools(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is not too early -- prepare shutdown is broadcasted before applications are stopped, as initlevel is decreased after broadcasting this event.
Therefore applications might be failing during their stop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree this is probably too early
* @author Jerome Dochez | ||
* @author Carla Mott | ||
* @AUTHOR: Jerome Dochez | ||
* @AUTHOR: Carla Mott |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: could you retain the cleanups from reverted commit, as these invalid javadocs, or typos further down the source?
pump = new Thread() { | ||
@Override | ||
public void run() { | ||
while (!done.isSignalled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There also was another branch in the newer version, flushing records if logToFile
is set to false in course of run. I believe that one should be retained
Complete change of subject. Did you look to see if GFFileHandler log pump could be refactored to not need its own thread? |
replaced by #4250 |
Description
This is a performance imporvement
reverts #3437
Due to the behaviour of GFFileHandler logpump it is better for it to be in it's own thread rather than use the executor service, having it as part of the executor service causes it to carry on running until killed.
Testing
New tests
None
Testing Performed
Ran the command
asadmin start-domain --verbose --debug && date +"%Y-%m-%d %H:%M:%S.%3N
to print out time with milliseconds after domain is stopped, and compared that time with time that server shutdown was started.Time before PR - 6.398 seconds
Time with 3c57d71 - 6.099 seconds
Time with c98a042 - 0.984 seconds
Test suites executed
Testing Environment
"Zulu JDK 1.8_222 on Ubuntu 19.04 DIsco Dingo with Maven 3.6.0"-->