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

update service status bug #3880

Closed
YyyQq-C opened this issue Sep 21, 2020 · 3 comments
Closed

update service status bug #3880

YyyQq-C opened this issue Sep 21, 2020 · 3 comments

Comments

@YyyQq-C
Copy link

@YyyQq-C YyyQq-C commented Sep 21, 2020

code bug

  • com.alibaba.nacos.api.naming.pojo.ServiceInfo#validate function's return always true.
    public boolean validate() {
        if (isAllIPs()) {
            return true;
        }

        List<Instance> validHosts = new ArrayList<Instance>();
        for (Instance host : hosts) {
            if (!host.isHealthy()) {
                continue;
            }

            for (int i = 0; i < host.getWeight(); i++) {
                validHosts.add(host);
            }
        }

        return true;
    }
  • com.alibaba.nacos.api.naming.pojo.ServiceInfo#getHosts function's return always not null
    public List<Instance> getHosts() {
        return new ArrayList<Instance>(hosts);
    }
  • com.alibaba.nacos.client.naming.core.HostReactor#processServiceJSON
100. public ServiceInfo processServiceJSON(String json) {
101.         ServiceInfo serviceInfo = JSON.parseObject(json, ServiceInfo.class);
102.         ServiceInfo oldService = serviceInfoMap.get(serviceInfo.getKey());
103.         if (serviceInfo.getHosts() == null || !serviceInfo.validate()) {
104.             //empty or error push, just ignore
105.             return oldService;
106.         }
107. 
108.         if (oldService != null) {
109.             if (oldService.getLastRefTime() > serviceInfo.getLastRefTime()) {
110.                 NAMING_LOGGER.warn("out of date data received, old-t: " + oldService.getLastRefTime()
111.                     + ", new-t: " + serviceInfo.getLastRefTime());
112.             }
113. 
114.             serviceInfoMap.put(serviceInfo.getKey(), serviceInfo);
115.             //.....
116.          }
117.         //....
118. }

line 103 judge always false.

@KomachiSion
Copy link
Collaborator

@KomachiSion KomachiSion commented Sep 21, 2020

I think the author want to implement protection for push empty, but don't implement completely.

I'm not sure whether is a bug. But it is not useful for now.

Loading

@horizonzy
Copy link
Collaborator

@horizonzy horizonzy commented Sep 21, 2020

I think the author want to implement protection for push empty, but don't implement completely.

I'm not sure whether is a bug. But it is not useful for now.

agree, the origin logic is not suitable, client's serviceInfo should be same as server, even though hosts is empty. the code should be clean maybe

Loading

@KomachiSion
Copy link
Collaborator

@KomachiSion KomachiSion commented Dec 21, 2020

I think we should add a properties to open push empty protection. And fix this part logic.

Loading

KomachiSion pushed a commit that referenced this issue Jan 12, 2021
* NamingService Client support pushEmptyProtection.

* check hosts is null. if null, also invalid.
@KomachiSion KomachiSion added this to the 1.4.1 milestone Jan 12, 2021
wjm0729 added a commit to wjm0729/nacos that referenced this issue Jan 13, 2021
…op-import-v1

* 'develop' of https://github.com/alibaba/nacos:
  naming push service support CSharp client (alibaba#4670)
  remove final
  [ISSUE-alibaba#3880] NamingService Client support pushEmptyProtection. (alibaba#4665)
  [ISSUE-alibaba#4631] Free credential instance when serverHttpAgent shutdown. (alibaba#4634)
  Add CSharp Client
  Just choose one between nacosDomain mode and servers mode.
  upgrade axios version to 0.21.1 (alibaba#4632)
  [ISSUE-alibaba#4631] Remove timer,Use ScheduledThreadPoolExecutor replaced. (alibaba#4635)
  fix: fix jraft response instance (alibaba#4644)
  For alibaba#4291, support remove old raft metadata
  change tokenValidityInSeconds default value
  remove JwtTokenUtils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants