AMBARI-17529. Ambari LogSearch REST Layer should not queue up requests that have already been made. (rnettleton)
Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/4066b7ef Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/4066b7ef Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/4066b7ef Branch: refs/heads/branch-feature-AMBARI-18634 Commit: 4066b7ef524e8dd7dad03b510e0b4792695c2598 Parents: 8f20af2 Author: Bob Nettleton <[email protected]> Authored: Mon Nov 7 14:31:17 2016 -0500 Committer: Bob Nettleton <[email protected]> Committed: Mon Nov 7 14:31:42 2016 -0500 ---------------------------------------------------------------------- .../logging/LogSearchDataRetrievalService.java | 124 ++++++++--- .../LogSearchDataRetrievalServiceTest.java | 209 ++++++++++++++++++- 2 files changed, 307 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/4066b7ef/ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java b/ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java index 4929747..1c135b2 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java @@ -19,6 +19,7 @@ package org.apache.ambari.server.controller.logging; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; +import com.google.common.collect.Sets; import com.google.common.util.concurrent.AbstractService; import com.google.inject.Inject; import org.apache.ambari.server.AmbariService; @@ -76,7 +77,7 @@ public class LogSearchDataRetrievalService extends AbstractService { private Cache<String, Set<String>> logFileNameCache; /** - * a Cache of host+component names to a generated URI that + * A Cache of host+component names to a generated URI that * can be used to access the "tail" of a given log file. * * This data is generated by ambari-server, but cached here to @@ -86,6 +87,16 @@ public class LogSearchDataRetrievalService extends AbstractService { private Cache<String, String> logFileTailURICache; /** + * A set that maintains the current requests being made, + * but not yet completed. This Set will be used to + * keep multiple requests from being made for the same + * host/component combination. + * + */ + private final Set<String> currentRequests = Sets.newConcurrentHashSet(); + + + /** * Executor instance to be used to run REST queries against * the LogSearch service. */ @@ -130,7 +141,7 @@ public class LogSearchDataRetrievalService extends AbstractService { * combination, or null if that object does not exist in the cache. */ public Set<String> getLogFileNames(String component, String host, String cluster) { - String key = generateKey(component, host); + final String key = generateKey(component, host); // check cache for data Set<String> cacheResult = @@ -140,9 +151,18 @@ public class LogSearchDataRetrievalService extends AbstractService { LOG.debug("LogFileNames result for key = {} found in cache", key); return cacheResult; } else { - // queue up a thread to make the LogSearch REST request to obtain this information - LOG.debug("LogFileNames result for key = {} not in cache, queueing up remote request", key); - startLogSearchFileNameRequest(host, component, cluster); + // queue up a thread to create the LogSearch REST request to obtain this information + if (currentRequests.contains(key)) { + LOG.debug("LogFileNames request has been made for key = {}, but not completed yet", key); + } else { + LOG.debug("LogFileNames result for key = {} not in cache, queueing up remote request", key); + // add request key to queue, to keep multiple copies of the same request from + // being submitted + currentRequests.add(key); + startLogSearchFileNameRequest(host, component, cluster); + } + + } return null; @@ -182,8 +202,27 @@ public class LogSearchDataRetrievalService extends AbstractService { this.loggingRequestHelperFactory = loggingRequestHelperFactory; } + /** + * This protected method provides a way for unit-tests to insert a + * mock executor for simpler unit-testing. + * + * @param executor an Executor instance + */ + protected void setExecutor(Executor executor) { + this.executor = executor; + } + + /** + * This protected method allows for simpler unit tests. + * + * @return the Set of current Requests that are not yet completed + */ + protected Set<String> getCurrentRequests() { + return currentRequests; + } + private void startLogSearchFileNameRequest(String host, String component, String cluster) { - executor.execute(new LogSearchFileNameRequestRunnable(host, component, cluster)); + executor.execute(new LogSearchFileNameRequestRunnable(host, component, cluster, logFileNameCache, currentRequests)); } private AmbariManagementController getController() { @@ -205,7 +244,7 @@ public class LogSearchDataRetrievalService extends AbstractService { * which can then be used for subsequent requests for the same data. * */ - private class LogSearchFileNameRequestRunnable implements Runnable { + static class LogSearchFileNameRequestRunnable implements Runnable { private final String host; @@ -213,33 +252,70 @@ public class LogSearchDataRetrievalService extends AbstractService { private final String cluster; - private LogSearchFileNameRequestRunnable(String host, String component, String cluster) { - this.host = host; + private final Set<String> currentRequests; + + private final Cache<String, Set<String>> logFileNameCache; + + private LoggingRequestHelperFactory loggingRequestHelperFactory; + + private AmbariManagementController controller; + + LogSearchFileNameRequestRunnable(String host, String component, String cluster, Cache<String, Set<String>> logFileNameCache, Set<String> currentRequests) { + this(host, component, cluster, logFileNameCache, currentRequests, new LoggingRequestHelperFactoryImpl(), AmbariServer.getController()); + } + + LogSearchFileNameRequestRunnable(String host, String component, String cluster, Cache<String, Set<String>> logFileNameCache, Set<String> currentRequests, + LoggingRequestHelperFactory loggingRequestHelperFactory, AmbariManagementController controller) { + this.host = host; this.component = component; this.cluster = cluster; + this.logFileNameCache = logFileNameCache; + this.currentRequests = currentRequests; + this.loggingRequestHelperFactory = loggingRequestHelperFactory; + this.controller = controller; } @Override public void run() { LOG.debug("LogSearchFileNameRequestRunnable: starting..."); - LoggingRequestHelper helper = - new LoggingRequestHelperFactoryImpl().getHelper(getController(), cluster); - - if (helper != null) { - // make request to LogSearch service - Set<String> logFileNamesResult = - helper.sendGetLogFileNamesRequest(component, host); - - // update the cache if result is available - if (logFileNamesResult != null) { - LOG.debug("LogSearchFileNameRequestRunnable: request was successful, updating cache"); - logFileNameCache.put(generateKey(component, host), logFileNamesResult); + try { + LoggingRequestHelper helper = + loggingRequestHelperFactory.getHelper(controller, cluster); + + if (helper != null) { + // make request to LogSearch service + Set<String> logFileNamesResult = + helper.sendGetLogFileNamesRequest(component, host); + + // update the cache if result is available + if (logFileNamesResult != null) { + LOG.debug("LogSearchFileNameRequestRunnable: request was successful, updating cache"); + final String key = generateKey(component, host); + // update cache with returned result + logFileNameCache.put(key, logFileNamesResult); + } else { + LOG.debug("LogSearchFileNameRequestRunnable: remote request was not successful"); + } } else { - LOG.debug("LogSearchFileNameRequestRunnable: remote request was not successful"); + LOG.debug("LogSearchFileNameRequestRunnable: request helper was null. This may mean that LogSearch is not available, or could be a potential connection problem."); } - } else { - LOG.debug("LogSearchFileNameRequestRunnable: request helper was null. This may mean that LogSearch is not available, or could be a potential connection problem."); + } finally { + // since request has completed (either successfully or not), + // remove this host/component key from the current requests + currentRequests.remove(generateKey(component, host)); } } + + protected void setLoggingRequestHelperFactory(LoggingRequestHelperFactory loggingRequestHelperFactory) { + this.loggingRequestHelperFactory = loggingRequestHelperFactory; + } + + protected void setAmbariManagementController(AmbariManagementController controller) { + this.controller = controller; + } + + } + + } http://git-wip-us.apache.org/repos/asf/ambari/blob/4066b7ef/ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java b/ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java index 97c59f8..b58350b 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java @@ -17,12 +17,20 @@ */ package org.apache.ambari.server.controller.logging; +import com.google.common.cache.Cache; +import org.apache.ambari.server.controller.AmbariManagementController; import org.easymock.EasyMockSupport; import org.junit.Test; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.Executor; + import static org.easymock.EasyMock.expect; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertEquals; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.isA; +import static org.junit.Assert.*; /** * This test verifies the basic behavior of the @@ -102,4 +110,201 @@ public class LogSearchDataRetrievalServiceTest { } + @Test + public void testGetLogFileNamesDefault() throws Exception { + final String expectedHostName = "c6401.ambari.apache.org"; + final String expectedComponentName = "DATANODE"; + final String expectedClusterName = "clusterone"; + + EasyMockSupport mockSupport = new EasyMockSupport(); + + LoggingRequestHelperFactory helperFactoryMock = + mockSupport.createMock(LoggingRequestHelperFactory.class); + + Executor executorMock = + mockSupport.createMock(Executor.class); + + // expect the executor to be called to execute the LogSearch request + executorMock.execute(isA(LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable.class)); + // executor should only be called once + expectLastCall().once(); + + mockSupport.replayAll(); + + LogSearchDataRetrievalService retrievalService = + new LogSearchDataRetrievalService(); + retrievalService.setLoggingRequestHelperFactory(helperFactoryMock); + // call the initialization routine called by the Google framework + retrievalService.doStart(); + retrievalService.setExecutor(executorMock); + + + assertEquals("Default request set should be empty", + 0, retrievalService.getCurrentRequests().size()); + + Set<String> resultSet = + retrievalService.getLogFileNames(expectedComponentName, expectedHostName, expectedClusterName); + + assertNull("Inital query on the retrieval service should be null, since cache is empty by default", + resultSet); + assertEquals("Incorrect number of entries in the current request set", + 1, retrievalService.getCurrentRequests().size()); + assertTrue("Incorrect HostComponent set on request set", + retrievalService.getCurrentRequests().contains(expectedComponentName + "+" + expectedHostName)); + + mockSupport.verifyAll(); + } + + @Test + public void testGetLogFileNamesIgnoreMultipleRequestsForSameHostComponent() throws Exception { + final String expectedHostName = "c6401.ambari.apache.org"; + final String expectedComponentName = "DATANODE"; + final String expectedClusterName = "clusterone"; + + EasyMockSupport mockSupport = new EasyMockSupport(); + + LoggingRequestHelperFactory helperFactoryMock = + mockSupport.createMock(LoggingRequestHelperFactory.class); + + Executor executorMock = + mockSupport.createMock(Executor.class); + + mockSupport.replayAll(); + + LogSearchDataRetrievalService retrievalService = + new LogSearchDataRetrievalService(); + retrievalService.setLoggingRequestHelperFactory(helperFactoryMock); + // call the initialization routine called by the Google framework + retrievalService.doStart(); + // there should be no expectations set on this mock + retrievalService.setExecutor(executorMock); + + // set the current requests to include this expected HostComponent + // this simulates the case where a request is ongoing for this HostComponent, + // but is not yet completed. + retrievalService.getCurrentRequests().add(expectedComponentName + "+" + expectedHostName); + + Set<String> resultSet = + retrievalService.getLogFileNames(expectedComponentName, expectedHostName, expectedClusterName); + + assertNull("Inital query on the retrieval service should be null, since cache is empty by default", + resultSet); + + mockSupport.verifyAll(); + } + + @Test + public void testRunnableWithSuccessfulCall() throws Exception { + final String expectedHostName = "c6401.ambari.apache.org"; + final String expectedComponentName = "DATANODE"; + final String expectedClusterName = "clusterone"; + final String expectedComponentAndHostName = expectedComponentName + "+" + expectedHostName; + + EasyMockSupport mockSupport = new EasyMockSupport(); + + LoggingRequestHelperFactory helperFactoryMock = + mockSupport.createMock(LoggingRequestHelperFactory.class); + AmbariManagementController controllerMock = + mockSupport.createMock(AmbariManagementController.class); + LoggingRequestHelper helperMock = + mockSupport.createMock(LoggingRequestHelper.class); + + Cache cacheMock = + mockSupport.createMock(Cache.class); + Set currentRequestsMock = + mockSupport.createMock(Set.class); + + expect(helperFactoryMock.getHelper(controllerMock, expectedClusterName)).andReturn(helperMock); + expect(helperMock.sendGetLogFileNamesRequest(expectedComponentName, expectedHostName)).andReturn(Collections.singleton("/this/is/just/a/test/directory")); + // expect that the results will be placed in the cache + cacheMock.put(expectedComponentAndHostName, Collections.singleton("/this/is/just/a/test/directory")); + // expect that the completed request is removed from the current request set + expect(currentRequestsMock.remove(expectedComponentAndHostName)).andReturn(true).once(); + + mockSupport.replayAll(); + + LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable loggingRunnable = + new LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable(expectedHostName, expectedComponentName, expectedClusterName, + cacheMock, currentRequestsMock, helperFactoryMock, controllerMock); + loggingRunnable.run(); + + mockSupport.verifyAll(); + + } + + @Test + public void testRunnableWithFailedCallNullHelper() throws Exception { + final String expectedHostName = "c6401.ambari.apache.org"; + final String expectedComponentName = "DATANODE"; + final String expectedClusterName = "clusterone"; + final String expectedComponentAndHostName = expectedComponentName + "+" + expectedHostName; + + EasyMockSupport mockSupport = new EasyMockSupport(); + + LoggingRequestHelperFactory helperFactoryMock = + mockSupport.createMock(LoggingRequestHelperFactory.class); + AmbariManagementController controllerMock = + mockSupport.createMock(AmbariManagementController.class); + + Cache cacheMock = + mockSupport.createMock(Cache.class); + Set currentRequestsMock = + mockSupport.createMock(Set.class); + + // return null to simulate an error during helper instance creation + expect(helperFactoryMock.getHelper(controllerMock, expectedClusterName)).andReturn(null); + // expect that the completed request is removed from the current request set, + // even in the event of a failure to obtain the LogSearch data + expect(currentRequestsMock.remove(expectedComponentAndHostName)).andReturn(true).once(); + + mockSupport.replayAll(); + + LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable loggingRunnable = + new LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable(expectedHostName, expectedComponentName, expectedClusterName, + cacheMock, currentRequestsMock, helperFactoryMock, controllerMock); + loggingRunnable.run(); + + mockSupport.verifyAll(); + + } + + @Test + public void testRunnableWithFailedCallNullResult() throws Exception { + final String expectedHostName = "c6401.ambari.apache.org"; + final String expectedComponentName = "DATANODE"; + final String expectedClusterName = "clusterone"; + final String expectedComponentAndHostName = expectedComponentName + "+" + expectedHostName; + + EasyMockSupport mockSupport = new EasyMockSupport(); + + LoggingRequestHelperFactory helperFactoryMock = + mockSupport.createMock(LoggingRequestHelperFactory.class); + AmbariManagementController controllerMock = + mockSupport.createMock(AmbariManagementController.class); + LoggingRequestHelper helperMock = + mockSupport.createMock(LoggingRequestHelper.class); + + Cache cacheMock = + mockSupport.createMock(Cache.class); + Set currentRequestsMock = + mockSupport.createMock(Set.class); + + expect(helperFactoryMock.getHelper(controllerMock, expectedClusterName)).andReturn(helperMock); + // return null to simulate an error occurring during the LogSearch data request + expect(helperMock.sendGetLogFileNamesRequest(expectedComponentName, expectedHostName)).andReturn(null); + // expect that the completed request is removed from the current request set, + // even in the event of a failure to obtain the LogSearch data + expect(currentRequestsMock.remove(expectedComponentAndHostName)).andReturn(true).once(); + + mockSupport.replayAll(); + + LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable loggingRunnable = + new LogSearchDataRetrievalService.LogSearchFileNameRequestRunnable(expectedHostName, expectedComponentName, expectedClusterName, + cacheMock, currentRequestsMock, helperFactoryMock, controllerMock); + loggingRunnable.run(); + + mockSupport.verifyAll(); + + } + }
