sofastack / sofa-rpc

SOFARPC is a high-performance, high-extensibility, production-level Java RPC framework.
https://www.sofastack.tech/sofa-rpc/docs/Home
Apache License 2.0
3.81k stars 1.17k forks source link

fix empty string ip("") #1298

Closed tcfly closed 11 months ago

tcfly commented 1 year ago

Motivation:

Explain the context, and why you're making that change. To make others understand what is the problem you're trying to solve.

Modification:

Describe the idea and modifications you've done.

Result:

Fixes #1297.

If there is no issue then describe the changes introduced by this PR.

sofastack-bot[bot] commented 1 year ago

Hi @tcfly, welcome to SOFAStack community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

codecov[bot] commented 1 year ago

Codecov Report

Merging #1298 (679eabc) into master (af35a32) will increase coverage by 0.05%. The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1298      +/-   ##
============================================
+ Coverage     71.96%   72.01%   +0.05%     
- Complexity      784      785       +1     
============================================
  Files           415      415              
  Lines         17651    17651              
  Branches       2753     2753              
============================================
+ Hits          12702    12712      +10     
+ Misses         3547     3534      -13     
- Partials       1402     1405       +3     
Impacted Files Coverage Δ
...ofa/rpc/registry/consul/HealthServiceInformer.java 84.78% <0.00%> (-4.35%) :arrow_down:
...n/java/com/alipay/sofa/rpc/log/TimeWaitLogger.java 80.00% <0.00%> (-4.00%) :arrow_down:
...com/alipay/sofa/rpc/context/RpcRuntimeContext.java 88.88% <0.00%> (-1.24%) :arrow_down:
...lipay/sofa/rpc/message/AbstractResponseFuture.java 56.14% <0.00%> (-0.88%) :arrow_down:
.../alipay/sofa/rpc/metrics/lookout/RpcLookoutId.java 88.88% <0.00%> (ø)
...va/com/alipay/sofa/rpc/client/AbstractCluster.java 71.80% <0.00%> (+0.26%) :arrow_up:
...fa/rpc/transport/triple/TripleClientTransport.java 71.66% <0.00%> (+0.83%) :arrow_up:
...pc/transport/NotReusableClientTransportHolder.java 63.63% <0.00%> (+3.03%) :arrow_up:
...ansport/bolt/ReuseBoltClientConnectionManager.java 72.46% <0.00%> (+7.24%) :arrow_up:
...java/com/alipay/sofa/rpc/module/LookoutModule.java 85.71% <0.00%> (+33.33%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nobodyiam commented 1 year ago

review again

nobodyiam commented 1 year ago

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 679eabcbde4cd6dc9684382cd529c69d4dc48cb1

The changes in this pull request are:

  1. In the file NacosRegistryHelper.java, the condition for checking if the host is empty has been changed from host == null to StringUtils.isEmpty(host).

Key findings:

  1. The change aims to address the empty string IP issue (i.e., "").
  2. The new condition prevents an empty host from being assigned as the server host. It will now fall back to SystemInfo.getLocalHost() if the host is either null or an empty string.

Potential problems:

  1. If there are other places in the code that expect the host to be null or an empty string, this change might have unintended consequences.
  2. It is not clear if there are any side-effects of using SystemInfo.getLocalHost() as a fallback in all cases.
  3. The changes might not cover all cases where IPs are generated; a comprehensive testing strategy is required to ensure that empty string IPs are not introduced in other parts of the code.
stale[bot] commented 12 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.