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

Document uses of eureka appName, vipAddress, with regards to spring cloud uses of serviceId, etc... #1788

Open
sfussenegger opened this issue Mar 17, 2017 · 7 comments

Comments

@sfussenegger
Copy link
Contributor

EurekaDiscoveryClient.getIntsances(String) doesn't use the serviceId or application name but the VIP Address to lookup instances:

	public List<ServiceInstance> getInstances(String serviceId) {
		List<InstanceInfo> infos = this.eurekaClient.getInstancesByVipAddress(serviceId,
				false);
		List<ServiceInstance> instances = new ArrayList<>();
		for (InstanceInfo info : infos) {
			instances.add(new EurekaServiceInstance(info));
		}
		return instances;
	}

here's the doc for DiscoverClient.getInstances(String serviceId)

	/**
	 * Get all ServiceInstances associated with a particular serviceId
	 * @param serviceId the serviceId to query
	 * @return a List of ServiceInstance
	 */
	List<ServiceInstance> getInstances(String serviceId);

So far I haven't noticed this as EurekaInstanceConfigBean uses spring.application.name as virtualHostName which is used by InstanceInfoFactory as VIPAddress

here's the doc for InstanceInfo:

    /**
     * Gets the Virtual Internet Protocol address for this instance. Defaults to
     * hostname if not specified.
     *
     * @return - The Virtual Internet Protocol address
     */
    @JsonProperty("vipAddress")
    public String getVIPAddress() {
        return vipAddress;
    }

        /**
         * Sets the Virtual Internet Protocol address for this instance. The
         * address should follow the format <code><hostname:port></code> This
         * address needs to be resolved into a real address for communicating
         * with this instance.
         *
         * @param vipAddress - The Virtual Internet Protocol address of this instance.
         * @return the instance builder.
         */
        public Builder setVIPAddress(String vipAddress) {
            result.vipAddressUnresolved = StringCache.intern(vipAddress);
            result.vipAddress = StringCache.intern(resolveDeploymentContextBasedVipAddresses(vipAddress));
            return this;
        }

so it looks like this implementation treats application name and VIP address as synonyms - and don't rely use VIP addresses anywhere - so that's why this bug went undetected.

So the proper implementation would be this:

	@Override
	public List<ServiceInstance> getInstances(String serviceId) {
		Application application = this.eurekaClient.getApplication(serviceId);
		List<ServiceInstance> instances = new ArrayList<>();
		if (application != null) {
			for (InstanceInfo info : application.getInstances()) {
				instances.add(new EurekaServiceInstance(info));
			}
		}
		return instances;
	}
@spencergibb
Copy link
Member

spencergibb commented Mar 17, 2017

I think, at most, there should be some clarifying documentation.

@spencergibb
Copy link
Member

Thinking of changing getInstances() to query using this.eurekaClient.getInstancesByVipAddressAndAppName(null, serviceId, false);. Thoughts, @sfussenegger? I know you've moved on from eureka.

@spencergibb
Copy link
Member

I think I used vip address for parity with the eureka ribbon integration.

@spencergibb
Copy link
Member

I think the best thing would be to document uses, see #1789 (comment)

@spencergibb spencergibb changed the title EurekaDiscoveryClient gets instances by VIP address Document uses of eureka appName, vipAddress, with regards to spring cloud uses of serviceId, etc... Aug 16, 2017
@spencergibb
Copy link
Member

More reasons for spring-cloud/spring-cloud-commons#227

@sfussenegger
Copy link
Contributor Author

@spencergibb it's been a while but I agree that really writing down the meaning of the terms application name, virtual host name, host name, instance id, and service id would go a long way towards solving the issue.

What's the difference though between eurekaClient.getInstancesByVipAddressAndAppName(null, serviceId, false) and eurekaClient.getApplication(serviceId) other than the secure flag? Both use serviceId to query for an appName so it looks like the approach I recommended at the end of my original comment.

I'm really somewhat removed from the whole Eureka and Ribbon thing. Though I do trust my 5-month-younger self enough to say that this seems to be a good idea ;)

@codefromthecrypt
Copy link

ps I lost a lot of time here, because there's no parity for reasons mentioned above. If you know the vip address, likely you have little to use eureka for. It seems odd that the service list returns applications (which makes sense), but you have to guess a vip address to well. discover metadata around that vip address. basically you already have to know a lot of info to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants