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-4355] Fix dubbo.jar do not contain "serialization-protobuf-json" module issue #4356

Merged
merged 3 commits into from
Jun 20, 2019

Conversation

vio-lin
Copy link
Contributor

@vio-lin vio-lin commented Jun 20, 2019

fix issue #4355

What is the purpose of the change

the value of include node in dubbo-all/pom.xml is different from value in module
org.apache.dubbo:dubbo-serialization-protobuf-json

org.apache.dubbo dubbo-serialization-protobuf-json compile true

Brief changelog

change one node value in dubbo-all/pom.xml

Verifying this change

change one node value in dubbo-all/pom.xml

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.
  • [x ] 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 sample in dubbo samples project.
  • 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.

@htynkn htynkn added this to the 2.7.3 milestone Jun 20, 2019
@htynkn htynkn self-assigned this Jun 20, 2019
@htynkn htynkn changed the title [Dubbo-2.7.3] fix dubbo.jar do not contain codes in "serialization-protobuf-json" module [Dubbo-4355] Fix dubbo.jar do not contain "serialization-protobuf-json" module issue Jun 20, 2019
@htynkn
Copy link
Member

htynkn commented Jun 20, 2019

LGTM, CI running now. will merge after CI pass. Close #4355

@htynkn htynkn merged commit ae22f9e into apache:2.7.3-release Jun 20, 2019
@codecov-io
Copy link

Codecov Report

Merging #4356 into 2.7.3-release will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##             2.7.3-release    #4356      +/-   ##
===================================================
- Coverage            62.97%   62.88%   -0.09%     
+ Complexity             505      503       -2     
===================================================
  Files                  769      769              
  Lines                32995    32995              
  Branches              5215     5215              
===================================================
- Hits                 20777    20749      -28     
- Misses                9822     9842      +20     
- Partials              2396     2404       +8
Impacted Files Coverage Δ Complexity Δ
.../apache/dubbo/qos/protocol/QosProtocolWrapper.java 64.1% <0%> (-17.95%) 0% <0%> (ø)
.../apache/dubbo/remoting/transport/AbstractPeer.java 63.04% <0%> (-10.87%) 0% <0%> (ø)
...apache/dubbo/common/config/ConfigurationUtils.java 64% <0%> (-8%) 0% <0%> (ø)
...ng/transport/dispatcher/all/AllChannelHandler.java 51.42% <0%> (-5.72%) 0% <0%> (ø)
.../org/apache/dubbo/remoting/ExecutionException.java 15.78% <0%> (-5.27%) 0% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyServer.java 69.64% <0%> (-3.58%) 8% <0%> (-1%)
...exchange/support/header/HeaderExchangeHandler.java 64.75% <0%> (-2.46%) 0% <0%> (ø)
...he/dubbo/registry/multicast/MulticastRegistry.java 67.87% <0%> (-1.81%) 0% <0%> (ø)
...pache/dubbo/remoting/transport/AbstractServer.java 47.91% <0%> (-1.05%) 0% <0%> (ø)
.../exchange/support/header/HeaderExchangeServer.java 66.98% <0%> (-0.95%) 0% <0%> (ø)
... and 2 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 041a6ad...430f293. Read the comment docs.

htynkn added a commit that referenced this pull request Jun 20, 2019
lovepoem pushed a commit that referenced this pull request Jun 21, 2019
…n" module issue (#4356) (#4364)

* include protobuf-json jar to dubbo
@vio-lin
Copy link
Contributor Author

vio-lin commented Jun 21, 2019

Hi all,
In this merge, some branch is commit information is not what i want. As my commit is merged into master but not in Release 2.7.3.If there is any wrong occurred. Time is 2019.6.21 12:18
图片
Dubbo-4355 should merged into 2.7.3-release, isn't it。

@htynkn
Copy link
Member

htynkn commented Jun 21, 2019

hey @vio-lin , I have to say sorry for this mistake.

First this PR should include in 2.7.3 release and thanks very much for your contribution. F
or version 2.7.3, we are not officially in code frenze for this version and also there are many chance to include it (for example in RC). And this PR is necessary for master branch too. So for this PR, it should go to master branch first and will be pick up into release branch.

So yesterday after I check this PR and click merge button in github, I notice this PR is for release branch, instead of master. I try to cherry-pick your commit to master and push it, but because of the configuration for repo, changes to master must be reviewed so I can't push directly. then I create another PR #4364 to merge it to master.
I thought in this way, the commit should able to keep your name and include in git history, but after click merge button, github do a squash and overwrite origin author, so the commit information is changed. Really very sorry for this mistake.

And for release branch, I revert it because changes should review and pick up when release process officially start. And I mentioned above, this change will include in 2.7.3 and I already added milestone label yesterday.

Thanks again for your contribution and sorry for this merge issue that changed commit information.

@vio-lin
Copy link
Contributor Author

vio-lin commented Jun 21, 2019

hey @vio-lin , I have to say sorry for this mistake.

First this PR should include in 2.7.3 release and thanks very much for your contribution. F
or version 2.7.3, we are not officially in code frenze for this version and also there are many chance to include it (for example in RC). And this PR is necessary for master branch too. So for this PR, it should go to master branch first and will be pick up into release branch.

So yesterday after I check this PR and click merge button in github, I notice this PR is for release branch, instead of master. I try to cherry-pick your commit to master and push it, but because of the configuration for repo, changes to master must be reviewed so I can't push directly. then I create another PR #4364 to merge it to master.
I thought in this way, the commit should able to keep your name and include in git history, but after click merge button, github do a squash and overwrite origin author, so the commit information is changed. Really very sorry for this mistake.

And for release branch, I revert it because changes should review and pick up when release process officially start. And I mentioned above, this change will include in 2.7.3 and I already added milestone label yesterday.

Thanks again for your contribution and sorry for this merge issue that changed commit information.

Hi htynkn ,
Thank you for your message. It is so grateful to receive such a long message with all information I need.
I think it is ok for me to continue working in branch 2.7.3 release.Thank you.

chickenlj pushed a commit that referenced this pull request Jun 24, 2019
…n" module issue (#4356) (#4364)

* include protobuf-json jar to dubbo
vio-lin pushed a commit to vio-lin/incubator-dubbo that referenced this pull request Jun 25, 2019
@vio-lin vio-lin deleted the changeProtobufJsonIncludeName branch June 25, 2019 11:25
rolandhe pushed a commit to rolandhe/dubbo that referenced this pull request Sep 9, 2019
rolandhe pushed a commit to rolandhe/dubbo that referenced this pull request Sep 9, 2019
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.

3 participants