> On July 1, 2016, 2:31 p.m., Jonathan Hurley wrote: > >
Thanks for the review! I'll be uploading an updated patch shortly with the fixes for these issues. > On July 1, 2016, 2:31 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProvider.java, > > lines 92-93 > > <https://reviews.apache.org/r/49474/diff/1/?file=1434489#file1434489line92> > > > > This is interesting; how would this service not be available if it's > > injected? There's no specific case that might cause this reference to be null. There are two reasons I left this check for null in the code: 1. I moved the access to the LoggingRequestHelper to the retrieval service, and so this change allowed me to keep the code changes to a minimum in order to introduce the new service. 2. I've noticed that our DI system depends a lot on object ordering, and I've seen instances in the past where an object is silently not injected, which could cause a NullPointerException. I'd like to keep this check in place, to guard against future object graph changes causing an NPE in this area of the code. > On July 1, 2016, 2:31 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java, > > line 79 > > <https://reviews.apache.org/r/49474/diff/1/?file=1434487#file1434487line79> > > > > Based on what we've seen, the requests to the LOGSEARCH endpoint don't > > return until the connection times out (which could be never depending on > > what's configured). This will then cause this Executor to fill it's queue > > with a backlog of requests. > > > > Instead, maybe it would be better to: > > - Only enqueue if the request isn't already enqueued > > - Use a bounded executor and rejection policy I agree with your suggestion, but would like to address this issue in a separate patch. I've filed the following JIRA to track this effort: https://issues.apache.org/jira/browse/AMBARI-17529 > On July 1, 2016, 2:31 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java, > > lines 149-151 > > <https://reviews.apache.org/r/49474/diff/1/?file=1434487#file1434487line149> > > > > Maybe only enqueue requests if the same request isn't on the queue. I agree with your suggestion, but would like to address this issue in a separate patch. I've filed the following JIRA to track this effort: https://issues.apache.org/jira/browse/AMBARI-17529 On July 1, 2016, 2:31 p.m., Robert Nettleton wrote: > > I'm a bit confused by the changes made here, namely that > > `sendLogLevelQueryRequest` was replaced with `sendGetLogFileNamesRequest`. > > Is `sendLogLevelQueryRequest` no longer used? If so, it can be removed, > > right? Thanks for catching this. The calls to sendLogLevelQueryRequest are removed, since the Ambari UI will not use these calls in Ambari 2.4. I left the underlying code in the LoggingRequestHelper's implementation, since I'd like to re-introduce this call (in a more performant way) in Ambari 2.5. Since that code is also unit-tested, it seemed to make sense to leave it in to be used in the next release. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49474/#review140353 ----------------------------------------------------------- On June 30, 2016, 9:11 p.m., Robert Nettleton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49474/ > ----------------------------------------------------------- > > (Updated June 30, 2016, 9:11 p.m.) > > > Review request for Ambari, Jonathan Hurley, Mahadev Konar, Oliver Szabo, and > Sumit Mohanty. > > > Bugs: AMBARI-17510 > https://issues.apache.org/jira/browse/AMBARI-17510 > > > Repository: ambari > > > Description > ------- > > This patch implements some updates to the Ambari LogSearch integration layer > in order to resolve AMBARI-17510. > > The initial implementation of the LoggingSearchPropertyProvider directly made > calls to the LogSearch Server's REST endpoint, in order to obtain LogSearch > metadata, and to attach this information to the Ambari HostComponent REST > resource. Recent testing has shown that there are some deployment scenarios > (larger clusters, rolling upgrade) that demonstrate a performance problem in > the LogSearch integration code. > > This patch addresses this problem by implementing the following: > > 1. A new, injectable service > (org.apache.ambari.server.controller.logging.LogSearchDataRetrievalService) > has been introduced into the LoggingSearchPropertyProvider. The property > provider now consults this service to obtain the required LogSearch data. > This new service is based on Jonathan Hurley's work in > org.apache.ambari.server.state.services.MetricsRetrievalService. > 2. The LogSearchDataRetrievalService utilizes an internal cache to maintain a > map of the LogSearch data, which typically will not change. If the cache > does not contain this information for a given HostComponent, the retrieval > service will make the required REST call to the LogSearch service to obtain > this data, and then add this to the cache. The remote REST call now happens > on a separate thread from the Ambari REST request handler, which removes the > possibility of causing degradation on the Ambari REST endpoing. > 3. The LoggingRequestHelperImpl class has been slightly updated, in order to > add 5 second connect/read timeouts on the LogSearch requests. This should > prevent any threads from hanging infinitely if the LogSearch server is slow > to respond. In a future patch, we might want to make these timeouts > configurable. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > 947a9f4 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > fe7e757 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractProviderModule.java > 5ac66d8 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java > d8c71e2 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProvider.java > ff7e7f5 > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProviderTest.java > 593f660 > > Diff: https://reviews.apache.org/r/49474/diff/ > > > Testing > ------- > > 1. Ran the ambari-server "mvn clean test" suite, and all Java and Python unit > tests passed with this change applied. > 2. Deployed a 3-node cluster with HDFS, Yarn, and LogSearch selected with > this patch applied. Verified that the cluster deployed successfully, and that > the "host_components" REST resource (the resource associated with this > performance problem) returns properly. Ran 6-7 separate REST clients making > concurrent requests on the "host_componets" resource in Ambari for over 40 > minutes, and verified that there is no noticeable slowdown. Verified that > ambari-server remains responsive after this test. Also verified via > DEBUG-level logging that the LogSearch remote REST requests are indeed > happening on a separate thread now. > 3. Deployed a 3-node cluster with HDFS and Yarn selected (LogSearch not > selected) with this patch applied. Verified that the cluster deploys > properly, and that the Ambari REST "host_components" resource returns > properly when LogSearch is not included in the cluster. > > > Thanks, > > Robert Nettleton > >
