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

[Dubbo-3886] Integrate with SOFARegistry. #3886

Merged
merged 9 commits into from
May 2, 2019

Conversation

ujjboy
Copy link
Contributor

@ujjboy ujjboy commented Apr 17, 2019

What is the purpose of the change

#3874

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

  • Make sure there is a GITHUB_issue field 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=false & 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.

@ujjboy ujjboy changed the title [WIP] Integrate with SOFARegistry. [Dubbo-3886] Integrate with SOFARegistry. Apr 17, 2019
@codecov-io
Copy link

codecov-io commented Apr 17, 2019

Codecov Report

Merging #3886 into master will increase coverage by 0.1%.
The diff coverage is 85.34%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #3886     +/-   ##
===========================================
+ Coverage     63.85%   63.95%   +0.1%     
  Complexity       98       98             
===========================================
  Files           715      718      +3     
  Lines         31457    31613    +156     
  Branches       5049     5090     +41     
===========================================
+ Hits          20086    20218    +132     
- Misses         9061     9078     +17     
- Partials       2310     2317      +7
Impacted Files Coverage Δ Complexity Δ
...che/dubbo/registry/sofa/SofaRegistryConstants.java 0% <0%> (ø) 0 <0> (?)
...pache/dubbo/registry/sofa/SofaRegistryFactory.java 100% <100%> (ø) 0 <0> (?)
...a/org/apache/dubbo/registry/sofa/SofaRegistry.java 85.58% <85.58%> (ø) 0 <0> (?)
...che/dubbo/remoting/transport/mina/MinaChannel.java 43.42% <0%> (-10.53%) 0% <0%> (ø)
...dubbo/common/status/support/LoadStatusChecker.java 46.15% <0%> (-3.85%) 0% <0%> (ø)
.../dubbo/monitor/support/AbstractMonitorFactory.java 78.37% <0%> (-2.71%) 0% <0%> (ø)
...ting/exchange/support/header/HeartbeatHandler.java 83.72% <0%> (-2%) 0% <0%> (ø)
.../remoting/transport/netty4/NettyClientHandler.java 68.42% <0%> (-1.23%) 0% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyClient.java 66.1% <0%> (-1.14%) 0% <0%> (ø)
...e/dubbo/remoting/transport/netty4/NettyClient.java 70.58% <0%> (-1.06%) 0% <0%> (ø)
... and 32 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 e189048...6e68107. Read the comment docs.

@ujjboy
Copy link
Contributor Author

ujjboy commented Apr 22, 2019

@ralf0131 Please review this PR.

@ralf0131
Copy link
Contributor

I will try to do it today or tomorrow..

Copy link
Contributor

@ralf0131 ralf0131 left a comment

Choose a reason for hiding this comment

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

Please following the guide to add a new module: apache/dubbo-website#350

There are some things to do:

  • Add your dependency to dubbo-bom/pom.xml
  • Add your dependency to dubbo-distribution/pom.xml
  • Update dubbo-dependencies if necessary
  • Could you list the license of your dependencies to make sure they are compatible with Apache

dubbo-all/pom.xml Outdated Show resolved Hide resolved
dubbo-all/pom.xml Outdated Show resolved Hide resolved
dubbo-registry/dubbo-registry-sofa/pom.xml Show resolved Hide resolved
dubbo-registry/dubbo-registry-sofa/pom.xml Show resolved Hide resolved
dubbo-rpc/dubbo-rpc-thrift/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@lovepoem lovepoem left a comment

Choose a reason for hiding this comment

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

Please use junit5

@ujjboy ujjboy force-pushed the integrate_with_sofa_registry branch from b5a6d77 to b5f8218 Compare April 26, 2019 12:17
@ujjboy ujjboy force-pushed the integrate_with_sofa_registry branch from 0b1ac21 to 3e6157c Compare April 27, 2019 12:05
@ujjboy
Copy link
Contributor Author

ujjboy commented Apr 27, 2019

@ralf0131 @lovepoem Hi, I have changed the code according to review, but CI only build error in openjdk11, Can you retry it?

Copy link
Contributor

@ralf0131 ralf0131 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ralf0131
Copy link
Contributor

@lovepoem What is you opinion on this?

@ralf0131
Copy link
Contributor

I checked the license of dubbo-registry-sofa dependencies. All looks good.

@lovepoem
Copy link
Member

lovepoem commented May 2, 2019

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants