> On Dec. 15, 2016, 3:04 p.m., Jonathan Hurley wrote: > >
Hi Jonathan, thanks for the review comments. > On Dec. 15, 2016, 3:04 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java, > > lines 5086-5090 > > <https://reviews.apache.org/r/54756/diff/1/?file=1584727#file1584727line5086> > > > > This seems like an anti-pattern which we should clean up. Doesn't have > > to be here though since the code that does this was simply moved here. I do agree that this is an anti-pattern, but unfortunately this appears to be the only way to support creating a BaseService subclass that requires dependency injection. I just looked over the other subtypes of BaseService, and none seem to require dependency injection as the LoggingService now does. My intent here was to gradually refactor the LogSearch integration code to utilize the DI framework more. In this particular case, the LoggingService was created manually, as were its dependencies, so I was trying to move towards using DI for the obvious benefits. I completely agree that this should be fixed, perhaps in a future version of Ambari, since there have been a few times where I've had to resort to this type of change, when in reality our entire framework should be using DI from the ground up. > 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? 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? - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54756/#review159298 ----------------------------------------------------------- On Dec. 14, 2016, 8:08 p.m., Robert Nettleton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54756/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2016, 8:08 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 > >
