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

Support for same host different ports apps #105

Open
wants to merge 2 commits into
base: 1.x
Choose a base branch
from

Conversation

codependent
Copy link

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

if(attributes != null && attributes.get("port") != null){
String port = attributes.get("port");
if(other.getAttributes() != null && other.getAttributes().get("port") != null){
equals &= (port == other.getAttributes().get("port"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is broken. You are comparing strings with "==". It will always be false.

@codependent
Copy link
Author

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
Copy link
Author

@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
Copy link

@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
Copy link
Author

@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
Copy link

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

@codependent
Copy link
Author

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
Copy link

stuartstevenson commented Nov 29, 2016

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;
        }```

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

Successfully merging this pull request may close these issues.

4 participants