Netflix / Turbine

SSE Stream Aggregator
Apache License 2.0
835 stars 255 forks source link

Support for same host different ports apps #105

Open codependent opened 8 years ago

codependent commented 8 years ago

This pull request solves the problem described in issue 88: being able to have different microservices deployed in the same host with different ports

codependent commented 8 years ago

Ups, what a rookie mistake, I was thinking it was an int. Anyways, I don't know if this is still necessary. I use Turbine in a Spring Cloud Netflix project and it was solved there.

This change (when fixed) would make their change redundant...what do you think?

codependent commented 8 years ago

@spencergibb @reimer-eimer Thinking twice about it... it would make sense that the solution were on Turbine's side. They fixed it in org/springframework/cloud/netflix/turbine/EurekaInstanceDiscovery.java, so it works when you have Eureka. but doesn't when using other discovery server such as Consul.

With this PR it would work with any Discovery Server. It would be neccesary to remove Spring Cloud Netflix's EurekaInstanceDiscovery.java changes though.

micheal-swiggs commented 8 years ago

@codependent I believe that it is not necessary to remove the changes to EurekaInstanceDiscovery.java as these changes are only used if turbine.combineHostPort = true.

codependent commented 8 years ago

@micheal-swiggs but having turbine.combineHostPort = true would never be necessary since turbine-core/src/main/java/com/netflix/turbine/monitor/instance/InstanceMonitor.java would always combine the host name and the port whenever there's a port:

public String getName() {
    String hostName = host.getHostname();
    if(this.host.getAttributes() != null && this.host.getAttributes().get("port") != null){
            hostName += ":" + this.host.getAttributes().get("port");
    }
    return hostName;
 }

So what would be the point of keeping it?

micheal-swiggs commented 8 years ago

Removing the current implementation would break it for those that are using it.

codependent commented 8 years ago

The current implementation isn't in a Spring Cloud RELEASE version but in a milestone, is it? I don't know Spring Cloud project policy but IMHO if it hasn't made it to a GA version it could be removed without even deprecating it.

As long as the next Spring Cloud Build/Milestone referenced a Turbine version that included this PR, nothing would break.

stuartstevenson commented 7 years ago

I found this method in InstanceUrlClosure.java that gives an example of how you can dynamically set the port if you are using your own InstanceDiscovery implementation


         * Replaces any {placeholder} attributes in a url suffix using instance attributes
         *
         * e.x. :{server-port}/hystrix.stream -> :8080/hystrix.stream
         *
         * @param host instance
         * @param url suffix
         * @return replaced url suffix
         */
        private String processAttributeReplacements(Instance host, String url) {
            for (Map.Entry<String, String> attribute : host.getAttributes().entrySet()) {
                String placeholder = "{"+attribute.getKey()+"}";
                if (url.contains(placeholder)) {
                    url = url.replace(placeholder, attribute.getValue());
                }
            }
            return url;
        }```