> On April 12, 2016, 9:24 a.m., Sebastian Toader wrote: > >
Hi Sebastian, Thanks for the review comments. Please see my replies below. Thanks, Bob > On April 12, 2016, 9:24 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImpl.java, > > lines 44-47 > > <https://reviews.apache.org/r/45979/diff/1/?file=1338249#file1338249line44> > > > > `ambariManagementController` is solely used to get `Clusters`. I think > > this method could look like `public LoggingRequestHelper getHelper(Clusters > > clusters, String clusterName)`. Probably writing a unit test for this > > method will be easier as setting up `AmbariManagementController` is more > > complicated than `Clusters`. > > > > Ideally this method should have only `Cluster` input param. I'd like to keep the reference to the AmbariManagementController in this factory's interface. Here's why: 1. This patch is the first of several to handle the integration components for LogSearch. I have another patch already that is using the controller interface for other things like credential management, which I plan to submit later this week. 2. While currently the LogSearch service only handles a single cluster, that assumption may change over time, so I'd rather let the factory be a little more flexible, in case LogSearch is modified to handle multiple cluster instances in Ambari. 3. Developing unit tests for this class was actually quite simple, and the controller interface didn't add much overhead with respect to test development, since I used EasyMock. Please see: ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImplTest.java for more details on that. > On April 12, 2016, 9:24 a.m., Sebastian Toader wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImpl.java, > > lines 75-77 > > <https://reviews.apache.org/r/45979/diff/1/?file=1338249#file1338249line75> > > > > If an error occurs while loading `Cluster` that could singal a fatal > > error that the application can not recover from. I think the exception > > should be re-thrown. > > > > Is it valid scenario to invoke this method for a wrong cluster and > > continue the running of the sever? I agree that better error-handling overall needs to be added to the integration code for LogSearch. In this particular case, I'm not sure that it is necessarily a fatal error if the factory is passed an invalid cluster name. That being said, I would prefer to have more straightforward error handling here, and in the LogSearch integration in general. I've filed a tracking JIRA to improve the error-handling in this area of the code, and I'll handle that in a subsequent patch: https://issues.apache.org/jira/browse/AMBARI-15834 Thanks. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45979/#review128352 ----------------------------------------------------------- On April 11, 2016, 4:27 p.m., Oliver Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45979/ > ----------------------------------------------------------- > > (Updated April 11, 2016, 4:27 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Jaimin > Jetly, Jayush Luniya, Robert Nettleton, Sumit Mohanty, Sebastian Toader, and > Yusaku Sako. > > > Bugs: AMBARI-15807 > https://issues.apache.org/jira/browse/AMBARI-15807 > > > Repository: ambari > > > Description > ------- > > Logsearch support was splitted into 3 different commits: > https://github.com/apache/ambari/tree/branch-dev-logsearch > - integrate logsearch module > - ambari server REST implementation for logsearch > - stack definition > > This one is the REST implementation. (by Robert Nettleton) > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/LoggingResourceDefinition.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ResourceInstanceFactoryImpl.java > c711bed > > ambari-server/src/main/java/org/apache/ambari/server/api/services/ClusterService.java > 371411d > > ambari-server/src/main/java/org/apache/ambari/server/api/services/LoggingService.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractControllerResourceProvider.java > f24da8d > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/AbstractProviderModule.java > ca491f2 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/LoggingResourceProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/HostComponentLoggingInfo.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogFileDefinitionInfo.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogFileType.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogLineResult.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LogQueryResponse.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelper.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactory.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProvider.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/spi/Resource.java > 63af4c4 > ambari-server/src/main/resources/key_properties.json 46a6cf9 > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogLineResultTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LogQueryResponseTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingRequestHelperFactoryImplTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/logging/LoggingSearchPropertyProviderTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/45979/diff/ > > > Testing > ------- > > Testing done. > > > Thanks, > > Oliver Szabo > >
