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

PAYARA-4118 Speedup Payara shutdown by having GFFileHandler use its o… #4250

Merged
merged 2 commits into from Oct 7, 2019

Conversation

@Cousjava
Copy link
Member

Cousjava commented Oct 4, 2019

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 4ddcf7f - 1.235 seconds

Test suites executed

  • Quicklook
  • Payara Samples
  • Java EE7 Samples
  • Java EE8 Samples
  • Payara Private Tests
  • Payara Microprofile TCKs Runner
  • Jakarta TCKs
  • Mojarra
  • Cargo Tracker

Testing Environment

"Zulu JDK 1.8_222 on Ubuntu 19.04 DIsco Dingo with Maven 3.6.0"-->

@Cousjava Cousjava added this to the 5.194 milestone Oct 4, 2019
@Cousjava Cousjava requested a review from pdudits Oct 4, 2019
@Cousjava Cousjava mentioned this pull request Oct 4, 2019
0 of 9 tasks complete
@Cousjava

This comment has been minimized.

Copy link
Member Author

Cousjava commented Oct 4, 2019

Jenkins test please

…wn thread

GFFileHandler no longer uses PayaraExecutorService
This is a partial revert of f60de6a / #3437
@Cousjava Cousjava force-pushed the Cousjava:PAYARA-4118-speedup-shutdown branch from 4ddcf7f to 5dc63c6 Oct 4, 2019
@Cousjava

This comment has been minimized.

Copy link
Member Author

Cousjava commented Oct 4, 2019

Jenkins test please

Copy link
Contributor

MarkWareham left a comment

Quick glance through. Should be good as a revert

@Pandrex247 Pandrex247 changed the title PAYARA-4118 Sppedup Payara shutdown by having GFFileHandler use its o… PAYARA-4118 Speedup Payara shutdown by having GFFileHandler use its o… Oct 7, 2019
drainAllPendingRecords();
flush();
Comment on lines -606 to -607

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 7, 2019

Contributor

What about this code branch? Shouldn't it remain in the block?

By the way using new Thread(Runnable) would allow you to retain original lambda.


public static LogRotationTimer getInstance() {
return instance;
}

public void startTimer(ScheduledExecutorService scheduledExecutorService, LogRotationTimerTask timerTask) {

This comment has been minimized.

Copy link
@pdudits

pdudits Oct 7, 2019

Contributor

On the other hand, this was task very fit for payara executor service, as it is very infrequent timed one

Copy link
Member

Pandrex247 left a comment

Don't really have a reproducer so can't test before vs. after, but just a minor comment.

pumpFuture = payaraExecutorService.submit(
() -> {
while (!done.isSignalled() && logToFile) {
pump = new Thread() {

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Oct 7, 2019

Member

It might be worth putting in a comment explaining why we're not using a the managed executor service here so that we don't come back to this in a year's time and go "why is that not using a managed executor?"

@Cousjava

This comment has been minimized.

Copy link
Member Author

Cousjava commented Oct 7, 2019

Jenkins test please

@Cousjava Cousjava merged commit ef7f590 into payara:master Oct 7, 2019
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.