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.

Sign up for GitHub

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

Jump to bottom

Issue #6020 Fix redeploy error jetty-maven-plugin #6023

Merged
merged 8 commits into from Mar 10, 2021
Merged

Issue #6020 Fix redeploy error jetty-maven-plugin #6023

merged 8 commits into from Mar 10, 2021

Conversation

janbartel
Copy link
Contributor

Closes #6020

  • Use TimerScheduler instead of ScheduledExecutorScheduler as the latter produces InterruptedException
  • Fix startup message of scan from ms to secs

@janbartel
Issue #6020 Fix redeploy error jetty-maven-plugin
075b21e 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel self-assigned this Mar 2, 2021
@janbartel janbartel added this to In progress in Jetty 10.0.2/11.0.2 via automation Mar 2, 2021
@janbartel janbartel mentioned this pull request Mar 2, 2021
Closed
@janbartel janbartel requested a review from sbordet March 3, 2021 10:27
Jetty 10.0.2/11.0.2 automation moved this from In progress to Review in progress Mar 3, 2021
sbordet
sbordet requested changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

One of the main points of the issue is to change the default for scan, and it's not been addressed?

jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java Outdated
@@ -516,7 +516,7 @@ public void doStart() throws Exception


//Create the scheduler and start it
_scheduler = new ScheduledExecutorScheduler("Scanner-" + SCANNER_IDS.getAndIncrement(), true, 1);
_scheduler = new TimerScheduler("Scanner-" + SCANNER_IDS.getAndIncrement(), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to move away from Timer as it's not configurable, has problems, etc. so this needs to go back to ScheduledExecutorScheduler.

Test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the ScheduledExecutorScheduler doesn't work: it interrupts its threads and throws the error you originally reported. The TimerScheduler doesn't manage its threads in that fashion and works just fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not fixed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to ScheduledExecutorScheduler - IMHO it's a bit ugly, but ....

I also think if you don't want TimerScheduler used, the class needs to be deprecated.

@janbartel
Issue 6020 Use scan=-1 for no hot redeployment.
00f40f7 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel requested a review from sbordet March 3, 2021 17:08
@janbartel
Copy link
Contributor Author

Introduced scan=-1 meaning that no redeployment is done, and made it the default. 0 means manual redeployment using Enter key, and any other positive integer is the scan interval for hot redeployment.

@janbartel
Copy link
Contributor Author

@sbordet nudge for re-review

sbordet
sbordet requested changes Mar 8, 2021
View reviewed changes
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc Outdated
By default, it will not automatically restart your webapp: you can force a redeploy by hitting the `Enter` key.
Set a non-zero &lt;scan&gt; value to have jetty scan your webapp for changes and automatically redeploy.
By default, it will not automatically restart your webapp.
Set a non-zero `&lt;scan&gt;` value to have jetty scan your webapp for changes and automatically redeploy, or set `&lt;scan&gt;` to `0` to cause manual redeployment by hitting the `Enter` key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace HTML entities with correspondent characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use kbd:[Enter] to indicate the "Enter" key -- it will render more nicely, see https://docs.asciidoctor.org/asciidoc/latest/syntax-quick-reference/#keyboard-button-and-menu-macros

jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java Outdated
@@ -516,7 +516,7 @@ public void doStart() throws Exception


//Create the scheduler and start it
_scheduler = new ScheduledExecutorScheduler("Scanner-" + SCANNER_IDS.getAndIncrement(), true, 1);
_scheduler = new TimerScheduler("Scanner-" + SCANNER_IDS.getAndIncrement(), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not fixed yet.

@janbartel
Issue #6020 Use ScheduledExecutorScheduler for Scanner
ca6e406 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
40f307b 
…6020-jetty-maven-plugin-restart-error
@janbartel
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
6478742 
…6020-jetty-maven-plugin-restart-error
@janbartel
Issue #6020 Fix multiple restart after change to pom.xml
0a8f9a6 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel requested a review from sbordet March 9, 2021 09:49
@janbartel
Issue #6020 update doco
b5cbf6c 
Signed-off-by: Jan Bartel <janb@webtide.com>
sbordet
sbordet requested changes Mar 9, 2021
View reviewed changes
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc Outdated
@@ -11,6 +11,8 @@
// ========================================================================
//

:experimental:
Copy link
Contributor

Choose a reason for hiding this comment

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

This attribute should be moved to the .asciidoctorconfig file.

@janbartel
Issue #6020 move location of :experimental: attribute
0234962 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel requested a review from sbordet March 9, 2021 16:01
Jetty 10.0.2/11.0.2 automation moved this from Review in progress to Reviewer approved Mar 10, 2021
@janbartel janbartel merged commit 56864bd into jetty-10.0.x Mar 10, 2021
Jetty 10.0.2/11.0.2 automation moved this from Reviewer approved to Done Mar 10, 2021
@janbartel janbartel deleted the jetty-10.0.x-6020-jetty-maven-plugin-restart-error branch March 10, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olamy olamy olamy approved these changes

@sbordet sbordet sbordet approved these changes

Assignees

@janbartel janbartel

Labels
None yet
Projects
No open projects
Jetty 10.0.2/11.0.2
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Review Jetty Maven Plugin scanning defaults
3 participants
@janbartel @olamy @sbordet