Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Fix for localhost metrics manager #2691

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrcrawfo
Copy link
Contributor

What?

  • Fixes for getting local hostname from within a Docker container

Why?

  • Metrics management doesn't work correctly if running Heron using Docker containers deployed in a cluster environment

@maosongfu
Copy link
Contributor

Two comments:

  1. Do you think we could make:
 if (System.getenv("HOST") != null) {
        this.hostname = System.getenv("HOST");
     } else {
        this.hostname = InetAddress.getLocalHost().getHostName();
     }

as an utlity method rather than duplicate the code multiple times?

  1. Use a more specific system env name like HERON_SERVER_HOST rather than the generic HOST to avoid any unexpected system injection?

@jrcrawfo
Copy link
Contributor Author

#1. Yes, we should do that. I’ll make that change.

#2. The reason for using HOST is because it is commonly passed in by the scheduler to be an actual resolvable hostname in a Docker environment (where hostname otherwise is the container id). So, it’s best to leave this as is, as there is also a helper function in the python executor code to do the same logic.

@maosongfu
Copy link
Contributor

@jrcrawfo
Fair. Will give a ship it once:

  1. The utility method is added.
  2. The Travis Ci build can pass.

@kramasamy
Copy link
Contributor

@jrcrawfo - any update on this PR?

@nlu90
Copy link
Member

nlu90 commented May 18, 2018

@jrcrawfo Ping for this PR, any updates?

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

Successfully merging this pull request may close these issues.

4 participants