> 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?
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.
- 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
>
>
