You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "saltyx (GitHub)" <gi...@apache.org> on 2019/03/06 07:59:00 UTC
[GitHub] [incubator-dubbo] saltyx opened pull request #3601: ConfigManger中的checkDuplicate方法逻辑反了
## What is the purpose of the change
checkDuplicate方法逻辑反了，导致校验配置无法通过
## Brief changelog
调整`!oldOne.equals(newOne)`为`oldOne.equals(newOne)`
## Verifying this change
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] Make sure there is a [GITHUB_issue](https://github.com/apache/incubator-dubbo/issues) 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](https://github.com/apache/incubator-dubbo/tree/master/dubbo-test).
- [ ] 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](https://github.com/apache/incubator-dubbo/wiki/Software-donation-guide).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] kezhenxu94 commented on issue #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "kezhenxu94 (GitHub)" <gi...@apache.org>.
@saltyx can you please also fix the failed CI build?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] saltyx commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "saltyx (GitHub)" <gi...@apache.org>.
I thought about it again, this code seems to be doing this deliberately. my logic is duplicate check and block the IllegalStateException, i should remove them .
And because `ConfigManager#checkDuplicate` is logically wrong, it will cause some unit test errors which is the purpose of changing this logic at the beginning...
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3601: Fix bug
of ConfigManger#checkDuplicate
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
`Two Configs with different are items are recognized as duplicate, that's why uses !oldOne.equals(newOne).`
Hi, Thank you for your explanation.
This is my mistake, I did not understand the meaning of this part of the code. I will remove my previous comments.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] saltyx commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "saltyx (GitHub)" <gi...@apache.org>.
such as AbstractInterfaceConfig#createApplicationIfAbsent , if `this.application` is null and the `ApplicationConfig` in `ConfigManager` is not null , it will reset `ApplicationConfig` variable，that always causes an error in `ConfigManager#checkDuplicate`
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #3601: ConfigManger中的checkDuplicate方法逻辑反了
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
LGTM
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] ralf0131 commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "ralf0131 (GitHub)" <gi...@apache.org>.
Still do not quite get it. in AbstractInterfaceConfig#createApplicationIfAbsent , if `this.application` is null, it will directly return.
Can you write a unit test to reproduce the error?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=h1) Report
> Merging [#3601](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/25f2d4c900b545ae32a61333637a5a2c375a05ba?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3601 +/- ##
============================================
- Coverage 64.26% 64.21% -0.05%
Complexity 71 71
============================================
Files 664 664
Lines 28653 28661 +8
Branches 4769 4773 +4
============================================
- Hits 18414 18406 -8
- Misses 8014 8035 +21
+ Partials 2225 2220 -5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...org/apache/dubbo/config/context/ConfigManager.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9jb250ZXh0L0NvbmZpZ01hbmFnZXIuamF2YQ==) | `65.07% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...g/apache/dubbo/config/AbstractInterfaceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9BYnN0cmFjdEludGVyZmFjZUNvbmZpZy5qYXZh) | `73.54% <75%> (-0.27%)` | `0 <0> (ø)` | |
| [...ache/dubbo/remoting/p2p/support/AbstractGroup.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctcDJwL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9wMnAvc3VwcG9ydC9BYnN0cmFjdEdyb3VwLmphdmE=) | `45.45% <0%> (-11.37%)` | `0% <0%> (ø)` | |
| [...org/apache/dubbo/rpc/protocol/AbstractInvoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JwYy9wcm90b2NvbC9BYnN0cmFjdEludm9rZXIuamF2YQ==) | `64.51% <0%> (-3.23%)` | `0% <0%> (ø)` | |
| [...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0NhbGxiYWNrU2VydmljZUNvZGVjLmphdmE=) | `76.47% <0%> (-2.21%)` | `0% <0%> (ø)` | |
| [...dubbo/remoting/exchange/support/DefaultFuture.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L0RlZmF1bHRGdXR1cmUuamF2YQ==) | `72.29% <0%> (-2.03%)` | `0% <0%> (ø)` | |
| [...a/org/apache/dubbo/monitor/dubbo/DubboMonitor.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tbW9uaXRvci9kdWJiby1tb25pdG9yLWRlZmF1bHQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL21vbml0b3IvZHViYm8vRHViYm9Nb25pdG9yLmphdmE=) | `87.85% <0%> (-1.87%)` | `0% <0%> (ø)` | |
| [...apache/dubbo/rpc/protocol/dubbo/DubboProtocol.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL0R1YmJvUHJvdG9jb2wuamF2YQ==) | `65.53% <0%> (-1.14%)` | `0% <0%> (ø)` | |
| [.../dubbo/registry/integration/RegistryDirectory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9pbnRlZ3JhdGlvbi9SZWdpc3RyeURpcmVjdG9yeS5qYXZh) | `80.18% <0%> (-0.93%)` | `0% <0%> (ø)` | |
| [...pache/dubbo/monitor/dubbo/DubboMonitorFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree#diff-ZHViYm8tbW9uaXRvci9kdWJiby1tb25pdG9yLWRlZmF1bHQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL21vbml0b3IvZHViYm8vRHViYm9Nb25pdG9yRmFjdG9yeS5qYXZh) | `100% <0%> (ø)` | `0% <0%> (ø)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3601/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=footer). Last update [25f2d4c...97eee4a](https://codecov.io/gh/apache/incubator-dubbo/pull/3601?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] saltyx commented on issue #3601: Fix bug
of ConfigManger#checkDuplicate
Posted by "saltyx (GitHub)" <gi...@apache.org>.
> @saltyx can you please also fix the failed CI build?
ok
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
@carryxyh Haha, it's ok, please feel free to express your opinions.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
@saltyx Sorry, I will close this PR now.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] saltyx commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "saltyx (GitHub)" <gi...@apache.org>.
ok
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj closed pull request #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
[ pull request closed by chickenlj ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] saltyx commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "saltyx (GitHub)" <gi...@apache.org>.
please check `AbstractInterfaceConfigTest#checkDuplicateApplication`
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] ralf0131 commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "ralf0131 (GitHub)" <gi...@apache.org>.
I notice the UT is missing here, could you add a unit test for this change?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] ralf0131 commented on pull request #3601:
Fix bug of ConfigManger#checkDuplicate
Posted by "ralf0131 (GitHub)" <gi...@apache.org>.
Why should we need this logic? The logic, including the null check and duplicate check have implemented in org.apache.dubbo.config.context.ConfigManager#setApplication. Why should be added it again?
So as the other change in this file.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] chickenlj commented on issue #3601: Fix
bug of ConfigManger#checkDuplicate
Posted by "chickenlj (GitHub)" <gi...@apache.org>.
Two Configs with different are items are recognized as duplicate, that's why uses `!oldOne.equals(newOne)`.
Two Configs with identically the same items are thought to be the same and one should be ignored silently.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3601 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org