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

Wrong port used to create DiscoveryNode #1

Closed
tjuchniewicz opened this issue Sep 14, 2016 · 26 comments · Fixed by #18
Closed

Wrong port used to create DiscoveryNode #1

tjuchniewicz opened this issue Sep 14, 2016 · 26 comments · Fixed by #18

Comments

@tjuchniewicz
Copy link
Contributor

tjuchniewicz commented Sep 14, 2016

Current code from EurekastOneDiscoveryStrategy:

InetAddress addr = mapAddress(instance);
int port = instance.getPort();
Map<String, Object> metadata = (Map) instance.getMetadata();
nodes.add(new SimpleDiscoveryNode(new Address(addr, port), metadata));

Problem is that port is eureka client port e.g. 8080 not Hazelcast port e.g. 5701.

I'm not sure if this is possible but this is my rough idea how to solve this problem or at least start discussion:

  1. After Hazelcast initialization eureka client adds Hazelcast port to eureka's metadata map.
  2. EurekastOneDiscoveryStrategy uses metadata map to get the port instead of instance.getPort();.
  3. Hazelcast must be initialized before Eureka Client?
@noctarius
Copy link
Contributor

Sorry for the late answer, not sure I got the explanation on how you think it can be fixed. Do you let Hazelcast register it itself?

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Feb 8, 2017

Yes. I let Hazelcast to register itself. Just after Hazelcast starts I put Hazelcast port number to Eureka instance metadata map. Metadata map is shared between nodes at every heartbeat. Then I read Hazelcast port of other nodes during discovery like this: https://github.com/ebjwc/eurekast-one/blob/gh-1/src/main/java/com/hazelcast/eurekast/one/EurekastOneDiscoveryStrategy.java#L131

@ghost
Copy link

ghost commented Jul 4, 2017

@tjuchniewicz Hi,

Is defined problem is still valid on your side? Do you need support? Also, please note that We have recently updated Eureka V1 discovery mechanism.

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Jul 4, 2017

Hi @lazerion
I didn't verify latest changes. I will try to switch to latest branch soon. Could you point out a code which fixes this issue?

@ghost
Copy link

ghost commented Jul 4, 2017

Hi @tjuchniewicz,

Current implementation and tests do not produce the problem currently. Could you please elaborate case, for instance are you overriding port in properties file?

Could you please check out the new version?

@tjuchniewicz
Copy link
Contributor Author

OK. I will prepare example project for this case.

@tjuchniewicz
Copy link
Contributor Author

Works for me now.
It is possible that last time I mixed-up something. I'm not sure now but maybe it worked all the time...

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Apr 4, 2018

Sorry but this wasn't fixed.
Hazelcast discovery works only in the case when hazelcast-eureka registers into Eureka but we have a bug here because hazelcast-eureka overrides in Eureka web application port with Hazelcast port!

Applications use two different ports for web and Hazelcast. In my solution I use Eureka metadata map to provide Hazelcast host and port used by application registered into Eureka. This requires some additional Eureka configuration in application:

// provide Hazelcast host and port in Eureka metadata
Map<String, String> map = applicationInfoManager.getInfo().getMetadata();
        
Member member = (Member) hazelcast.getLocalEndpoint();
Address address = member.getAddress();
String host = address.getHost();
int port = address.getPort();
        
map.put(EurekaHazelcastMetadata.HAZELCAST_HOST, host);
map.put(EurekaHazelcastMetadata.HAZELCAST_PORT, Integer.toString(port));

and then I use this information in hazelcast-eureka discovery code:

// read Hazelcast host and port from Eureka metadata
Map<String, Object> metadata = (Map) instance.getMetadata();
if (metadata.containsKey(EurekaHazelcastMetadata.HAZELCAST_PORT)) {
    port = Integer.parseInt(metadata.get(EurekaHazelcastMetadata.HAZELCAST_PORT).toString());
}
if (metadata.containsKey(EurekaHazelcastMetadata.HAZELCAST_HOST)) {
    try {
        address = InetAddress.getByName(metadata.get(EurekaHazelcastMetadata.HAZELCAST_HOST).toString());
    } catch (UnknownHostException e) {
        getLogger().warning("Instance address '" + instance + "' could not be resolved", e);
    }
}
nodes.add(new SimpleDiscoveryNode(new Address(address, port), metadata));

@googlielmo
Copy link
Contributor

Hi @tjuchniewicz

Can you elaborate a bit about your use case and how to reproduce the issue?

Points that are not really clear to me:

  • Are you using Hazelcast embedded in a web application?
  • If so, are you using the same Eureka client both for Hazelcast cluster member discovery and for another (logically independent) custom application discovery?

Thanks,

Guglielmo

@tjuchniewicz
Copy link
Contributor Author

Hi @googlielmo

  • Yes, I use Hazelcast embedded in a web application
  • Yes, I use the same Eureka client for both

I will prepare example how I use this now.

@tjuchniewicz
Copy link
Contributor Author

@googlielmo
Copy link
Contributor

Hi @tjuchniewicz

Thank you for contributing to Hazelcast.

If I understand correctly, I think the issue you are experiencing (i.e., finding the wrong port) stems from using one Eureka client for two "Applications" (in the Eureka sense), as follows:

  1. A Hazelcast instance connects to Eureka and registers under namespace NS1, with name NAME host HOST and port 5701.

  2. Then a web application (Spring Boot, but can be anything) connects to Eureka and registers under namespace NS1, with name NAME host HOST and port 8080.

Now, both Hazelcast and the web application live in the same process and are using the same client instance, which is configured to use app name NAME in namespace NS1.

The Discovery Plugin, which uses the same client again, requests a list of application from the same namespace and name and gets a list of all the instances of application NAME in namespace NS1.

So depending on the order of registration, for app NAME in namespace NS1 for host HOST it will only get one port, either 5701 or 8080.

(By the way, if you pass a Eureka client instance to EurekaOneDiscoveryStrategyFactory.setEurekaClient the configuration in hazelcast.xml, including the namespace, will be ignored.)

Now, I don't know how common this use case is: normally, I would recommend to use a separate client for Hazelcast Discovery, one that points to the same Eureka server instance, but with a different namespace (e.g., "hazelcast") and/or a different name.

What do you think?

@leszko
Copy link

leszko commented Jun 7, 2018

I understand the issue similar to @googlielmo, meaning that in your example [1], your namespace in hazelcast.xml is ignored and therefore you use the same namespace for both Spring Boot and Hazelcast. How about creating a second Eureka Bean here [2] (different than the default one you inject with @Autowired?

[1] https://github.com/tjuchniewicz/hazelcast-eureka-examples
[2] https://github.com/tjuchniewicz/hazelcast-eureka-examples/blob/master/hazelcast-node/src/main/java/test/hazelcast/HazelcastConfiguration.java

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Jun 12, 2018

Thanks for analysis and opinions @googlielmo and @leszko. Now I understand current implementation idea! I was missing the point all the time!

So we have two possible solutions for Hazelcast - Eureka integration:

  1. Create separate Eureka client for Hazelcast (we will get two Eureka applications, one for web and one for Hazelcast)
  2. Re-use existing Eureka client and enhance it's metadata map with Hazelcast specific metadata.

I believe (and this is how we use Eureka in our environment and probably Netflix does) 2. option is more common for our case.

My pros and cons:

  • The most important disadvantage of solution 1. is that we get two separate applications in Eureka registry. There is no separate application in this case. This is still the same application + Hazelcast. For example in spring-boot-admin you will see two entries for the same app.
  • This is related to Spring Boot but it's just easier to enhance existing client than create new one. In Spring Boot/Spring Cloud client already exists (@Autowired EurekaClient eurekaClient). We don't want hazelcast-eureka to create Eureka client because we want to share the same configuration but at the same time we don't want to provide second/copy configuration file. Existing Eureka client metadata enhancement may be implemented as a Spring Boot plugin for hazelcast-eureka.

I think we should support both scenarios. Current scenario is definitely required for the cases when Eureka is used only to build Hazelcast cluster. If web application cluster is already created we should reuse it.

@leszko
Copy link

leszko commented Jun 14, 2018

Hi @tjuchniewicz ,

Thanks for your explanation. I agree we could support both approaches, however I just have a couple of more questions. Note that I'm not Eureka expert, so some of them might by obvious :)

  1. Where is the Hazelcast Node (hazelcast.host, hazelcast.host, hazelcast.groupName) registered in the Metadata Map? In your PR I see you read these values, but never write them? Is it done automatically?
  2. Why do we need hazelcast.host? Isn't it the same as the address [1] always the same as address [2]?
  3. Is it a common practice to do it this way? How do you know that Netflix does it this way?

[1] https://github.com/hazelcast/hazelcast-eureka/pull/18/files#diff-497d97735c08391fca6a2598564e3599R261
[2] https://github.com/hazelcast/hazelcast-eureka/pull/18/files#diff-497d97735c08391fca6a2598564e3599R275

@tjuchniewicz
Copy link
Contributor Author

Hi @leszko

  1. Data is written to metadata map here: https://github.com/tjuchniewicz/hazelcast-eureka-examples/blob/master/hazelcast-node/src/main/java/test/hazelcast/HazelcastConfiguration.java
    This configuration class (should be rewritten for better integration with Spring) should be a part of new module hazelcast-eureka-spring. Our implementation:
@Configuration
@AutoConfigureAfter(value = HazelcastConfiguration.class)
@ConditionalOnBean(HazelcastInstance.class)
public class HazelcastToEurekaPropagationConfiguration {

    @Bean
    public HazelcastEurekaMetadataMapInfo updateEurekaMetadataMap(HazelcastInstance hazelcast,
            ApplicationInfoManager applicationInfoManager) {

        return new HazelcastEurekaMetadataMapInfo(hazelcast, applicationInfoManager);
    }
}

public class HazelcastEurekaMetadataMapInfo {
    
    public HazelcastEurekaMetadataMapInfo(HazelcastInstance hazelcast, ApplicationInfoManager applicationInfoManager) {
      
        log.info("Updating Eureka metadata map with Hazelcast info");
        Map<String, String> map = applicationInfoManager.getInfo().getMetadata();
        
        Member member = (Member) hazelcast.getLocalEndpoint();
        Address address = member.getAddress();
        String host = address.getHost();
        int port = address.getPort();
        
        log.info("Hazelcast member address: {}", address);
        
        map.put(EurekaHazelcastMetadata.HAZELCAST_HOST, host);
        map.put(EurekaHazelcastMetadata.HAZELCAST_PORT, Integer.toString(port));
        map.put(EurekaHazelcastMetadata.HAZELCAST_GROUP_NAME, hazelcast.getConfig().getGroupConfig().getName());
    }
}
  1. Yes this is duplication. My idea was to keep all Hazelcast node coordinates in one place even if we duplicate hostname.
  2. There is no direct proof. From: https://github.com/Netflix/eureka/wiki/Eureka-at-a-glance#how-is-eureka-used-at-netflix.

How is Eureka used at Netflix?
Used for carrying other additional application specific metadata about services for various other reasons.

@googlielmo
Copy link
Contributor

Hi @tjuchniewicz

I see your point, at the same time I think that if we want to have an alternative way to store Hazelcast info in Eureka, i.e., in metadata, then the discovery plugin should be configured both to write and read those metadata.

I don’t want to depend on some spring magic, because the Eureka discovery plugin is meant to be used in a variety of settings, e.g., in a standalone Hazelcast cluster, in non-spring apps, etc.

What I suggest is that you introduce a new boolean flag in the plugin config, say "USE_METADATA_FOR_HOST_AND_PORT" (default false, please come up with a better name if you wish), so that when the flag is true:

  1. when the node registers itself on Eureka it will do so with metadata, not host and port, and
  2. when it looks for other nodes it will use the metadata

Thoughts?

@tjuchniewicz
Copy link
Contributor Author

Hi @googlielmo That's perfect for me! I will improve my PR after my holidays.

@leszko
Copy link

leszko commented Jun 25, 2018

Perfect. We definitely need to provide both: write & read, not only read.

I'd consider if we need a flag at all. Or maybe a node can do both at the same time: always register in metadata AND host+port. And metadata takes the precedence. @googlielmo @tjuchniewicz, Wdyt?

@googlielmo
Copy link
Contributor

Not sure, if user wants to use host+port for their (other) App maybe the setting both will happen at the wrong time and override the other App's values ?

@leszko
Copy link

leszko commented Jun 25, 2018

You mean that in the case from @tjuchniewicz , Spring overrode Hazelcast, but Hazelcast may override Spring as well? I'm not sure it's an issue, because Hazelcast uses different namespace. Or I am missing something?

@googlielmo
Copy link
Contributor

Not sure either: My understanding was that if someone wanted to use host+port for other purposes (e.g., for their spring app) then the plugin should not interfere and just use the metadata.
This behaviour would be enabled through a new flag.
But let's hear from @tjuchniewicz as I may be missing some of the implications :-)

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Jun 25, 2018

You are right @googlielmo. This is exactly my idea.
@leszko We don't want to see two Eureka apps in registry even if they use different namespace. Our understanding is than one application instance (one JVM) should register only once in Eureka. We can use namespaces to group different apps (e.g. monitoring, ui, etc) but we don't want to split single JVM app into two Eureka apps.

@leszko
Copy link

leszko commented Jun 26, 2018

ok, it makes sense. Thanks @tjuchniewicz @googlielmo for the explanation!

@tjuchniewicz
Copy link
Contributor Author

tjuchniewicz commented Aug 18, 2018

Hi guys. I've updated my PR. Please take a look: #18

@googlielmo
Copy link
Contributor

Thanks for the update @tjuchniewicz! We'll have a look ASAP.

@mesutcelik mesutcelik modified the milestones: 1.0.3, 1.1 Sep 18, 2018
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.

5 participants