> On Dec. 15, 2016, 3:04 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java, > > lines 73-74 > > <https://reviews.apache.org/r/54756/diff/1/?file=1584729#file1584729line73> > > > > This is a little odd; you're injecting an injector to be able to get an > > instance of LoggingRequestHelperFactory from the injector... > > > > Why not just @Inject the LoggingREquestHelperFactory? > > Robert Nettleton wrote: > Yes, this does look a little strange. I chose this approach because this > service handles some LogSearch REST requests on a separate thread, and so I > wanted to inject a separate factory instance to the new task. The injected > factory helper impl is used on the main thread, to handle some > non-network-related URI calculations that can be returned to the caller > immediately. > > In this case, the intent was to have the DI handle the creation of the > object used on the main thread, and to use the injector for creating new > instances for the network-related requests. It seemed awkward to use the > injector to create the data member instance, but we could do that if it is > clearer. > > In the future, it would be nice to ensure the thread-safety of the > factory object, and just pass around a single injected instance, but my > thinking was that this would be out of scope for this bug fix. > > I'm thinking that I could just add some javadoc to clarify this further. > Would that be sufficient? > > Jonathan Hurley wrote: > Javadoc would be sufficient, yes. One thing I'm not clear on still is the > multi-threaded aspect. Isn't a new service instantiated per REST request? If > so, it's it bound to the thread from Jetty anyway? > > Robert Nettleton wrote: > As I understood it, any services annotated with "@AmbariService" are > singletons, and so my understanding is that the instance is not paired with > the bound thread. > > The usage of the injector is to create a new factory instance for each > new task launched in a separate thread. Here's the usage: > > {code} > private void startLogSearchFileNameRequest(String host, String component, > String cluster) { > executor.execute(new LogSearchFileNameRequestRunnable(host, > component, cluster, logFileNameCache, currentRequests, > > injector.getInstance(LoggingRequestHelperFactory.class))); > } > {code} > > In the future, it might be better to have the Runnable instances > themselves created by the injector, and just specify the dependencies there. > > I'll add some javadoc to make this clearer, and update the patch in this > review. > > Thanks. > > Jonathan Hurley wrote: > Ah, I confused this "Service" with "LoggingService" which is instantiated > each time it's asked for. Yes, this service is a singleton by nature of it > being an AmbariService. At the end of the day, you need a new instance each > time - I get that, no problem. I think the right way would be to inject a > factory into this class and have the factory give you your instances. > > With that said, if you need this in sooner rather than later, you can doc > this and then create a new Jira to track the addition of the factory.
I've just updated an updated patch to add javadoc to clarify this in the short-term, and I've created a JIRA to track this refactoring in the long-term: https://issues.apache.org/jira/browse/AMBARI-19217 Thanks. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54756/#review159298 ----------------------------------------------------------- On Dec. 15, 2016, 7:54 p.m., Robert Nettleton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54756/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2016, 7:54 p.m.) > > > Review request for Ambari, Jonathan Hurley, Miklos Gergely, Oliver Szabo, and > Sumit Mohanty. > > > Bugs: AMBARI-19105 > https://issues.apache.org/jira/browse/AMBARI-19105 > > > Repository: ambari > > > Description > ------- > > This patch resolves AMBARI-19105. > > Previously, the Ambari LogSearch integration used a hard-coded 5-second > timeout for its connection to the LogSearch Portal service. Since this may > not be optimal for various cluster sizes, new configuration properties have > been introduced to control this behavior. The default connect and read > timeouts are still 5 seconds. > > This patch implements the following: > 1. Adds two new properties to ambari.properties to control the connect and > read timeouts for an HTTP/S connection from the Ambari Server to the > LogSearch portal. > 2. Updates the LogSearch integration layer to use these timeouts when > establishing a connection to the LogSearch service. Some additional > refactoring was implemented, in order to take better advantage of the > dependency injection system used in Ambari. > 3. Updates existing unit tests, and adds new tests as well. > 4. Some basic code cleanup, removed dead code, etc. > 5. Fixes a small NullPointerException in the LogSearch code, including a new > unit test to verify this change. > > > Diffs > ----- > > ambari-server/docs/configuration/index.md 6ff263c > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > 072c4a2 > > ambari-server/src/main/java/org/apache/ambari/server/api/services/LoggingService.java > ea4960f > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java > f9b6878 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > 389f973 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 6b5731c > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/LoggingResourceProvider.java > 2eb1a63 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalService.java > e65cd59 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImpl.java > afe1757 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java > 88996d7 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProvider.java > 6ffcdf9 > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/Utils.java > fdc9267 > > ambari-server/src/test/java/org/apache/ambari/server/api/services/LoggingServiceTest.java > 64fff1e > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogSearchDataRetrievalServiceTest.java > 0bd681b > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImplTest.java > 7c8405d > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImplTest.java > 3129f6e > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/UtilsTest.java > 63b46ac > > Diff: https://reviews.apache.org/r/54756/diff/ > > > Testing > ------- > > 1. Manually verified this change against a local 3-node vagrant cluster. > 2. Ran the ambari-server "mvn clean test" suite with my patch applied. > > There were some failures in the python suite: > > "Total run:1154 > Total errors:93 > Total failures:1 > ERROR > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD FAILURE > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 25:58.648s > [INFO] Finished at: Wed Dec 14 14:03:10 EST 2016 > [INFO] Final Memory: 65M/1233M > [INFO] > ------------------------------------------------------------------------ > " > > I ran the full "mvn clean test" suite again without my changes applied, and > the same failures occurred, so my patch does not appear to cause this build > breakage. > > > Thanks, > > Robert Nettleton > >
