Skip to content

[Dubbo-1665] Elegant shutdown under tomcat #1665#1763

Merged
ralf0131 merged 8 commits intoapache:masterfrom
ralf0131:graceful-shutdown-in-tomcat
May 17, 2018
Merged

[Dubbo-1665] Elegant shutdown under tomcat #1665#1763
ralf0131 merged 8 commits intoapache:masterfrom
ralf0131:graceful-shutdown-in-tomcat

Conversation

@ralf0131
Copy link
Copy Markdown
Contributor

@ralf0131 ralf0131 commented May 9, 2018

What is the purpose of the change

Fix the issue that Dubbo cannot shutdown elegantly.

Brief changelog

  • Introduce a Bootstrap class to easily start/stop Dubbo when running without Spring.
  • Introduce DubboApplicationListener to listen to the Spring lifecycle event to start/stop Dubbo under Spring.

Verifying this change

  • Verified under embedded mode (without Spring) and under servlet container.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn clean install -DskipTests & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

@ralf0131 ralf0131 force-pushed the graceful-shutdown-in-tomcat branch from a3f2cea to 40596b0 Compare May 10, 2018 08:17
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 10, 2018 •
edited
Loading

Codecov Report

Merging #1763 into master will decrease coverage by 0.02%.
The diff coverage is 29.16%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
.../java/com/alibaba/dubbo/config/AbstractConfig.java 71.68% <ø> (-0.15%) 115 <0> (ø)
.../java/com/alibaba/dubbo/config/ProtocolConfig.java 74.61% <ø> (-0.39%) 59 <0> (-4)
.../java/com/alibaba/dubbo/config/RegistryConfig.java 37.64% <0%> (ø) 26 <0> (ø) ⬇️
...ng/initializer/DubboWebApplicationInitializer.java 0% <0%> (ø) 0 <0> (?)
...g/spring/initializer/DubboApplicationListener.java 0% <0%> (ø) 0 <0> (?)
...a/com/alibaba/dubbo/common/utils/ExecutorUtil.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ba/dubbo/registry/multicast/MulticastRegistry.java 44.76% <100%> (+0.23%) 28 <0> (ø) ⬇️
...om/alibaba/dubbo/registry/redis/RedisRegistry.java 32.77% <100%> (+0.18%) 16 <0> (ø) ⬇️
...om/alibaba/dubbo/registry/dubbo/DubboRegistry.java 46.15% <100%> (+1.05%) 8 <1> (ø) ⬇️
...ibaba/dubbo/registry/support/FailbackRegistry.java 55.11% <50%> (-0.22%) 33 <0> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf58e49...40596b0. Read the comment docs.

@chickenlj
Copy link
Copy Markdown
Contributor

chickenlj commented May 17, 2018

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:

  1. ReferenceConfig should also involved in DubboBootstrap, also consider ReferenceConfigCache.
  2. Make more configuration available in DubboBootstrap.
  3. Refactor the ServiceConfig to better cooperate with DubboBootstrap, for example add fluent builder for XxxConfig, support default ApplicationConfig.
  4. Provide guide demo introducing usage of DubboBootstrap.

chickenlj
chickenlj reviewed May 17, 2018
View reviewed changes
...rc/main/java/com/alibaba/dubbo/config/spring/initializer/DubboWebApplicationInitializer.java
public class DubboWebApplicationInitializer extends AbstractContextLoaderInitializer {

/**
* This method won't be triggered if running on spring-boot.
Copy link
Copy Markdown
Contributor

@chickenlj chickenlj May 17, 2018

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?

Copy link
Copy Markdown
Contributor Author

@ralf0131 ralf0131 May 17, 2018

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.

@ralf0131
Copy link
Copy Markdown
Contributor Author

ralf0131 commented May 17, 2018

@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.

@chickenlj
Copy link
Copy Markdown
Contributor

chickenlj commented May 17, 2018

@ralf0131 I think it's ready to merge, please solve the conflicts first.

@ralf0131 ralf0131 force-pushed the graceful-shutdown-in-tomcat branch from 40596b0 to e201004 Compare May 17, 2018 08:01
@ralf0131 ralf0131 merged commit e201004 into apache:master May 17, 2018
@ralf0131 ralf0131 deleted the graceful-shutdown-in-tomcat branch May 17, 2018 08:20
This was referenced May 17, 2018
Closed
Closed
Closed
Closed
Merged
@chickenlj chickenlj added this to the 2.6.4 milestone Jul 26, 2018
@diecui1202 diecui1202 removed this from the 2.6.4 milestone Sep 5, 2018
@chickenlj chickenlj mentioned this pull request Nov 2, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chickenlj chickenlj chickenlj left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ralf0131 @codecov-io @chickenlj @ATAKing1023 @diecui1202