try to add a better localhost resolver. See issue #44 #1
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package com.aphyr.riemann.client; | ||
|
|
||
| import java.net.UnknownHostException; | ||
|
|
||
| /** | ||
| * A "Smarter" localhost resolver | ||
| * see issue: https://github.com/aphyr/riemann-java-client/issues/44 | ||
| * Trying to avoid a lot of calls to java.net.InetAddress.getLocalHost() | ||
| * which under AWS trigger DNS resolving and have relatively high latency *per event* | ||
| * usually, the hostname doesn't change so often to warrant a real query. | ||
| * | ||
| * A real call to java.net.InetAddress.getLocalHost().getHostName() | ||
| * is made only if: | ||
| * 1) the refresh interval has passed (=result is stale) | ||
| * AND | ||
| * 2) no env vars that identify the hostname are found | ||
| */ | ||
| public class LocalhostResolver { | ||
|
|
||
| // default hostname env var names on Win/Nix | ||
| public static final String COMPUTERNAME = "COMPUTERNAME"; // Windows | ||
| public static final String HOSTNAME = "HOSTNAME"; // Nix | ||
|
|
||
| // how often should we refresh the cached hostname | ||
| public static long RefreshIntervalMillis = 60 * 1000; | ||
|
|
||
| // enables setting a custom env var used for resolving | ||
| public static String CustomEnvVarName = null; | ||
|
There was a problem hiding this comment. Too much customization. Would you ever need this? There was a problem hiding this comment. I thought for testing/staging or in cases where you couldn't change the usually reserved env vars. There was a problem hiding this comment. You could do that with custom injection points in the dev code. Anyway, I think it's not a requirement now. |
||
|
|
||
| // cached hostname result | ||
| private static String hostname; | ||
|
|
||
| // update (refresh) time management | ||
| private static long lastUpdate = 0; | ||
| private static long lastNetUpdate = 0; | ||
| public static long getLastUpdateTime() { return lastUpdate; } | ||
| public static long getLastNetUpdateTime() { return lastNetUpdate; } | ||
| public static void resetUpdateTimes() { | ||
| lastUpdate = 0; | ||
| lastNetUpdate = 0; | ||
| } | ||
|
|
||
| /** | ||
| * get resolved hostname. | ||
| * encapsulates all lookup and caching logic. | ||
| * | ||
| * @return the hostname | ||
| */ | ||
| public static String getResolvedHostname() { | ||
| long now = System.currentTimeMillis(); | ||
| if(now - RefreshIntervalMillis > lastUpdate) { | ||
| refreshResolve(); | ||
| } | ||
|
|
||
| return hostname; | ||
| } | ||
|
|
||
| /** | ||
| * forces a new resolve even if refresh interval has not passed yet | ||
| */ | ||
| public static void refreshResolve() { | ||
| try { | ||
| hostname = resolveByEnv(); | ||
|
There was a problem hiding this comment. this shouldn't be here. There is no need to resolve the environment variable again and again. There is also no need to call System.currentTimeMillis() each time in this case. |
||
| if(hostname == null || hostname.isEmpty()) { | ||
| hostname = java.net.InetAddress.getLocalHost().getHostName(); | ||
| lastNetUpdate = System.currentTimeMillis(); | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| //e.printStackTrace(); | ||
|
There was a problem hiding this comment. replace comment with // fallthrough |
||
| } | ||
| finally { | ||
| lastUpdate = System.currentTimeMillis(); | ||
|
|
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * try to resolve the hostname by env vars | ||
| * | ||
| * @return | ||
| */ | ||
| private static String resolveByEnv() { | ||
| if(CustomEnvVarName != null) { | ||
|
|
||
| return System.getenv(CustomEnvVarName); | ||
| } | ||
|
|
||
| if(System.getProperty("os.name").startsWith("Windows")) { | ||
| return System.getenv(COMPUTERNAME); | ||
|
There was a problem hiding this comment. use getenv instead of System.getenv which inludes an override for testing There was a problem hiding this comment. Not sure what you mean, I assume it relates to setEnv. Let's talk about the approach. There was a problem hiding this comment. As we talked, you suggested that adding an injection point in the dev code is not needed since we have the test code that can inject directly into env variables. |
||
| } | ||
|
|
||
| return System.getenv(HOSTNAME); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| package riemann.java.client.tests; | ||
|
|
||
| import com.aphyr.riemann.client.LocalhostResolver; | ||
| import junit.framework.Assert; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.net.UnknownHostException; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class LocalhostResolveTest { | ||
|
|
||
| protected Map<String, String> env = new HashMap<String, String>(); | ||
|
|
||
| protected static final String OS_NAME_PROP = "os.name"; | ||
| protected static final String ExpectedWinHostname = "LocalHostResolverTestWin"; | ||
|
There was a problem hiding this comment. change everything to private. no need for protected |
||
| protected static final String ExpectedNixHostname = "LocalHostResolverTestNix"; | ||
|
|
||
| @BeforeClass | ||
| public static void oneTimeSetUp() { | ||
| LocalhostResolver.RefreshIntervalMillis = 1000; | ||
| } | ||
|
|
||
| @Before | ||
| public void setup() { | ||
| env = new HashMap<String, String>(System.getenv()); | ||
| env.put(LocalhostResolver.HOSTNAME, ExpectedNixHostname); | ||
| env.put(LocalhostResolver.COMPUTERNAME, ExpectedWinHostname); | ||
| setEnv(env); | ||
|
|
||
| LocalhostResolver.CustomEnvVarName = null; | ||
| LocalhostResolver.resetUpdateTimes(); | ||
| Assert.assertEquals(0, LocalhostResolver.getLastUpdateTime()); | ||
| Assert.assertEquals(0, LocalhostResolver.getLastNetUpdateTime()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUpdateInterval() { | ||
| Assert.assertEquals(0, LocalhostResolver.getLastUpdateTime()); | ||
| String hostname = LocalhostResolver.getResolvedHostname(); | ||
| long lastUpdateTime = LocalhostResolver.getLastUpdateTime(); | ||
| Assert.assertNotNull(hostname); | ||
| try { | ||
| Thread.sleep(1500); | ||
|
There was a problem hiding this comment. you shouldn't use sleep in unit tests. You can expose get/setLastNetUpdateTime() for testing. |
||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| Assert.assertNotSame(lastUpdateTime, LocalhostResolver.getLastUpdateTime()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNoEnvVars() throws UnknownHostException { | ||
| env.remove(LocalhostResolver.HOSTNAME); | ||
| env.remove(LocalhostResolver.COMPUTERNAME); | ||
| setEnv(env); | ||
|
|
||
| String hostname = LocalhostResolver.getResolvedHostname(); | ||
| Assert.assertNotNull(hostname); | ||
|
|
||
| try { | ||
| Thread.sleep(1500); | ||
| } catch (InterruptedException e) { | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| // ensure queried hostname without env vars | ||
| Assert.assertEquals(LocalhostResolver.getLastUpdateTime(), LocalhostResolver.getLastNetUpdateTime()); | ||
| Assert.assertEquals(java.net.InetAddress.getLocalHost().getHostName(), hostname); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEnvVarWindows() { | ||
| System.getProperties().put(OS_NAME_PROP, "Windows7"); | ||
|
|
||
| String hostname = LocalhostResolver.getResolvedHostname(); | ||
| Assert.assertEquals(ExpectedWinHostname, hostname); | ||
| Assert.assertEquals(0, LocalhostResolver.getLastNetUpdateTime()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEnvVarNix() { | ||
| System.getProperties().put(OS_NAME_PROP, "Linux"); | ||
| String hostname = LocalhostResolver.getResolvedHostname(); | ||
| Assert.assertEquals(ExpectedNixHostname, hostname); | ||
| Assert.assertEquals(0, LocalhostResolver.getLastNetUpdateTime()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCustomEnvVar() { | ||
| final String customHostnameEnvVar = "AWS_HOST"; | ||
| final String customHostname = "EC2-LocalHostResolverTest"; | ||
| env.put(customHostnameEnvVar, customHostname); | ||
| setEnv(env); | ||
|
|
||
| LocalhostResolver.CustomEnvVarName = customHostnameEnvVar; | ||
| String hostname = LocalhostResolver.getResolvedHostname(); | ||
| Assert.assertEquals(customHostname, hostname); | ||
| Assert.assertEquals(0, LocalhostResolver.getLastNetUpdateTime()); | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * evil hack for testing (only!) with env var in-memory modification | ||
| * see: http://stackoverflow.com/a/7201825/1469004 | ||
| * | ||
| * @param newEnv - to set in memory | ||
| */ | ||
| protected static void setEnv(Map<String, String> newEnv) { | ||
| try { | ||
|
There was a problem hiding this comment. why??? Just add setEnv() in the production code and document it's a testing hook (which overrides getEnv()). There was a problem hiding this comment. Let's talk about this, not sure why you prefer this approach. |
||
| Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment"); | ||
| Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment"); | ||
| theEnvironmentField.setAccessible(true); | ||
| Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null); | ||
| env.putAll(newEnv); | ||
| Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment"); | ||
| theCaseInsensitiveEnvironmentField.setAccessible(true); | ||
| Map<String, String> cienv = (Map<String, String>) theCaseInsensitiveEnvironmentField.get(null); | ||
| cienv.putAll(newEnv); | ||
| } | ||
| catch (NoSuchFieldException e) { | ||
| try { | ||
| Class[] classes = Collections.class.getDeclaredClasses(); | ||
| Map<String, String> env = System.getenv(); | ||
| for(Class cl : classes) { | ||
| if("java.util.Collections$UnmodifiableMap".equals(cl.getName())) { | ||
| Field field = cl.getDeclaredField("m"); | ||
| field.setAccessible(true); | ||
| Object obj = field.get(env); | ||
| Map<String, String> map = (Map<String, String>) obj; | ||
| map.clear(); | ||
| map.putAll(newEnv); | ||
| } | ||
| } | ||
| } catch (Exception e2) { | ||
| e2.printStackTrace(); | ||
|
There was a problem hiding this comment. but then I have to make every test method declare throw (which I prefer not to here, since it's a hacky test method anyway). There was a problem hiding this comment. If you have Guava in the pom use throw Throwables.propagate(e2) otherwise use throw new RuntimeException(e2) |
||
| } | ||
| } catch (Exception e1) { | ||
| e1.printStackTrace(); | ||
|
|
||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these changes related to this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not, possibly new intellij version did this when importing the maven project.
otherwise it wouldn't recognize imports etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. so please consider reverting these file changes