-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
[Dubbo-1665] Elegant shutdown under tomcat #1665 #1763
Conversation
a3f2cea
to
40596b0
Compare
Codecov Report
@@ Coverage Diff @@
## master #1763 +/- ##
============================================
- Coverage 38.77% 38.74% -0.03%
+ Complexity 4141 4137 -4
============================================
Files 618 620 +2
Lines 29728 29725 -3
Branches 5241 5239 -2
============================================
- Hits 11527 11517 -10
- Misses 16360 16367 +7
Partials 1841 1841
Continue to review full report at Codecov.
|
I think this is a very good idea refactoring the startup and stop phase of dubbo process. At present, we need a place to highlight this kind of changes, so that users and contributors can easily get noticed and accept the new implementation, for this change, it's especially important for API users. Further consideration, can discuss later:
|
public class DubboWebApplicationInitializer extends AbstractContextLoaderInitializer { | ||
|
||
/** | ||
* This method won't be triggered if running on spring-boot. |
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.
If use spring-boot, will DubboApplicationListener still get registered?
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.
No. It is only intended for Spring running under servlet container.
For spring-boot support, I am planning to add the support in spring-boot-starter-project.
@chickenlj Yes, the current DubboBootstrap class only contains minimal functionality to ensure it works to solve this particular issue, I don't want to bring too much change to this pull request, we need add more later. |
@ralf0131 I think it's ready to merge, please solve the conflicts first. |
40596b0
to
e201004
Compare
What is the purpose of the change
Fix the issue that Dubbo cannot shutdown elegantly.
Brief changelog
Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX
. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests
&mvn clean test-compile failsafe:integration-test
to make sure unit-test and integration-test pass.