You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "khanimteyaz (GitHub)" <gi...@apache.org> on 2018/12/31 14:53:18 UTC

[GitHub] [incubator-dubbo] khanimteyaz opened pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

In this PR I have refactored code to

1.  Replace hard coded values from java code to constant. Most of this values where getting used from multiple places so moving to constant avoid development time or run time error for value mismatch.
2.  Small refactoring to reduce code volume and use existing JDK provided utility and operation.


 Bellow are the java file which has been modified as part of this PR

  dubbo-common/src/main/java/org/apache/dubbo/common/Constants.java
  dubbo-common/src/main/java/org/apache/dubbo/common/EnvironmentConstants.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractServiceConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ApplicationConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ConfigCenterConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/MethodConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ModuleConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ProtocolConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ProviderConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/RegistryConfig.java
  dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java

## What is the purpose of the change

1.  Replace hard coded values from java code to constant. Most of this values where getting used from multiple places so moving to constant avoid development time or run time error for value mismatch.
2.  Small refactoring to reduce code volume and use existing JDK provided utility and operation.

## Brief changelog

Hard coded value replacement of dubbo-config-api and small refactoring to optimize code size.

## Verifying this change

Running UT

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

- [x] Make sure there is a [3107](https://github.com/apache/incubator-dubbo/issues/3107) 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/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
Leaving the changing of java class to interface change. Or if you feel we should go ahead and make change please let the comment I will make the changes as part of this PR only.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
sure.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] code4wt commented on pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "code4wt (GitHub)" <gi...@apache.org>.
Hi, this code style looks a bit strange, I suggest writing the comma after the variable, like this:

```java
throw new IllegalStateException(String.format("Unsupported environment: %s, only support %s/%s/%s, default is %s.",
        environment,
        EnvironmentConstants.DEVELOPMENT_ENVIRONMENT,
        EnvironmentConstants.TEST_ENVIRONMENT,
        EnvironmentConstants.PRODUCTION_ENVIRONMENT,
        EnvironmentConstants.PRODUCTION_ENVIRONMENT));
```

What do you think?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=h1) Report
> Merging [#3108](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/d41408e23f9f0b26f19a7c3e8f8bcfb5e9aaed1f?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `68.53%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3108      +/-   ##
============================================
+ Coverage     63.52%   63.54%   +0.02%     
  Complexity       75       75              
============================================
  Files           652      652              
  Lines         28183    28188       +5     
  Branches       4791     4784       -7     
============================================
+ Hits          17902    17913      +11     
+ Misses         8023     8014       -9     
- Partials       2258     2261       +3
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/dubbo/common/Constants.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vQ29uc3RhbnRzLmphdmE=) | `92.85% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...va/org/apache/dubbo/config/ConfigCenterConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Db25maWdDZW50ZXJDb25maWcuamF2YQ==) | `31.03% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...org/apache/dubbo/config/AbstractServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9BYnN0cmFjdFNlcnZpY2VDb25maWcuamF2YQ==) | `95.77% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ava/org/apache/dubbo/config/ApplicationConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9BcHBsaWNhdGlvbkNvbmZpZy5qYXZh) | `84.33% <100%> (+1.62%)` | `0 <0> (ø)` | :arrow_down: |
| [...ain/java/org/apache/dubbo/config/ModuleConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Nb2R1bGVDb25maWcuamF2YQ==) | `86.84% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ain/java/org/apache/dubbo/config/MethodConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9NZXRob2RDb25maWcuamF2YQ==) | `100% <100%> (+1.72%)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/ProviderConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Qcm92aWRlckNvbmZpZy5qYXZh) | `95.69% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/RegistryConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWdpc3RyeUNvbmZpZy5qYXZh) | `84.21% <40%> (+1.85%)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/ProtocolConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Qcm90b2NvbENvbmZpZy5qYXZh) | `75.93% <42.85%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...in/java/org/apache/dubbo/config/ServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9TZXJ2aWNlQ29uZmlnLmphdmE=) | `53.74% <61.53%> (+0.39%)` | `0 <0> (ø)` | :arrow_down: |
| ... and [12 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?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/3108?src=pr&el=footer). Last update [d41408e...9ccc95a](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?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/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=h1) Report
> Merging [#3108](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/d41408e23f9f0b26f19a7c3e8f8bcfb5e9aaed1f?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `67.77%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3108      +/-   ##
============================================
+ Coverage     63.52%   63.56%   +0.03%     
  Complexity       75       75              
============================================
  Files           652      653       +1     
  Lines         28183    28189       +6     
  Branches       4791     4784       -7     
============================================
+ Hits          17902    17917      +15     
+ Misses         8023     8014       -9     
  Partials       2258     2258
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/org/apache/dubbo/common/Constants.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vQ29uc3RhbnRzLmphdmE=) | `92.85% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [.../org/apache/dubbo/common/EnvironmentConstants.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vRW52aXJvbm1lbnRDb25zdGFudHMuamF2YQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [...va/org/apache/dubbo/config/ConfigCenterConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Db25maWdDZW50ZXJDb25maWcuamF2YQ==) | `31.03% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...org/apache/dubbo/config/AbstractServiceConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9BYnN0cmFjdFNlcnZpY2VDb25maWcuamF2YQ==) | `95.77% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ava/org/apache/dubbo/config/ApplicationConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9BcHBsaWNhdGlvbkNvbmZpZy5qYXZh) | `84.33% <100%> (+1.62%)` | `0 <0> (ø)` | :arrow_down: |
| [...ain/java/org/apache/dubbo/config/ModuleConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Nb2R1bGVDb25maWcuamF2YQ==) | `86.84% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ain/java/org/apache/dubbo/config/MethodConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9NZXRob2RDb25maWcuamF2YQ==) | `100% <100%> (+1.72%)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/ProviderConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Qcm92aWRlckNvbmZpZy5qYXZh) | `95.69% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/RegistryConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9SZWdpc3RyeUNvbmZpZy5qYXZh) | `84.21% <40%> (+1.85%)` | `0 <0> (ø)` | :arrow_down: |
| [...n/java/org/apache/dubbo/config/ProtocolConfig.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnL2R1YmJvLWNvbmZpZy1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZy9Qcm90b2NvbENvbmZpZy5qYXZh) | `75.93% <42.85%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| ... and [18 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3108/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?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/3108?src=pr&el=footer). Last update [d41408e...1338133](https://codecov.io/gh/apache/incubator-dubbo/pull/3108?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/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 closed pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
On it. One quick check, if I change the Constants to an interface and dubbo ecosystem projects(outside of incubator-dubbo) are using this class by extending then they might start failing. Do you know any one of them using it? If no one is using then we can change it to interface now. 

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on pull request #3108: Replace hard coded dubbo-config-api values and small code optimization

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
suggest to put all constants in Constants, and make it an interface. After that, let's revisit this class to consider if it is possible to split. What do you think?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3108 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org