ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jonathan Hurley <jhur...@hortonworks.com>
Subject Re: Review Request 43621: Decrease the load on ambari database after cluster creation
Date Fri, 19 Feb 2016 18:15:06 GMT


> On Feb. 18, 2016, 2:27 p.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java,
lines 177-198
> > <https://reviews.apache.org/r/43621/diff/2/?file=1254367#file1254367line177>
> >
> >     LoadingCache interprets `null` as an actual value, meaning that if you were
to return `null`, it would cache it. 
> >     
> >     Instead, regardless of ID, you're returning a value anyway from `loadAggregateCounts`.
This means that no matter what ID I pass in, a value will be cached. 
> >     
> >     In theory, I could make queries for IDs which don't exist and pre-populate this
cache with stale, empty references. Perhaps it's better to return NULL when the request ID
has no data from `loadAggregateCounts` and then throw an exception from the `load` method.
This is the correct workflow to signal that a value requested doesn't exist and that the invalid
value should not be cached.
> 
> Sebastian Toader wrote:
>     It is a valid scenario to pass in a request ID for which an empty result is returned.
For example when you create a cluster using blueprints there will be a request with no HRCs
created until hosts join the cluster (the request is in Pending Host Group assignment state).
The loadAggregateCounts() method uses DaoUtils.selectList() which never returns null. If no
records returned by the query the returns and empty collection.
>     
>     
>     As for invalid request ids I agree that it should be signaled but that check shouldn't
made be here, rather ensure that caller invoked the findAggregateCounts method with valid
existing request id. Presently all methods invoking findAggregateCounts have a RequestEnity
they pick the request id from thus I think we can assume that all these call findAggregateCounts
with a valid request id.
>     
>     Of course if there is a bug in the caller than in may happen that a request id which
doesn't exists is passed in. If we want to be protected againts this than in the findAggregateCounts
method should query the db for the passed request id to check if exists and throw an exception
if it doesn't. Do you think still worth doing this (every time findAggregateCounts is called
it will make a round trip to the db to verify the request id) instead of ensuring in the callers
that valid request ids are being used?

Good point about the DaoUtils.selectList(). What I was trying to guard against here is the
null-cache situation. If requesting something that doesn't exist, returning a value for it,
even null, causes it to be cached. The proper workflow, according to Guava, is to throw an
exception from inside of the load() method.

But I think you're right; requests that don't exist is an edge case, and we don't want to
incur an extra JPA penalty for looking up by ID. Although, this shouldn't in theory, be a
round trip since JPA caches by ID, it's still something that probably isn't necessary.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43621/#review119688
-----------------------------------------------------------


On Feb. 19, 2016, 9:35 a.m., Sebastian Toader wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43621/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2016, 9:35 a.m.)
> 
> 
> Review request for Ambari, Jonathan Hurley, Myroslav Papirkovskyy, Sumit Mohanty, and
Sid Wagle.
> 
> 
> Bugs: AMBARI-15011
>     https://issues.apache.org/jira/browse/AMBARI-15011
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> 1. Created new index on host_role_command table on (status, role) fields as this table
is being queries quite often by status and role
> 2. Removed uneccessary querying of ambari server actions by hostname
> 3. Cache HostRoleCommandStatusSummaryDTO computed objects using guava cache
> 4. The method returning service configs made one call to the database to get the active
service config entities. Then retrieved from database all service config entities and while
building the response it was using the first collection to determine which reponse item should
be marked as active/inactive. This has been modified to make only one roundtrip to the database
and determine the active flag using plain java code.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
91f2d30 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java
d2fe4fc 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java
bd5fb5a 
>   ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java
bb8ba19 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java
20cf5bb 
>   ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java
d3fdc65 
>   ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog222.java
88b3151 
>   ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 18d3c84 
>   ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql d8a6e67 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql fbf68db 
>   ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql ac2fadf 
>   ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql cb5f5e3 
>   ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 7085a2d 
>   ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java
bc4d397 
>   ambari-server/src/test/java/org/apache/ambari/server/agent/AgentResourceTest.java 510e1fb

>   ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
1f90813 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
862776f 
>   ambari-server/src/test/java/org/apache/ambari/server/state/ConfigHelperTest.java 0cdf50a

>   ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog222Test.java
aa4090e 
>   ambari-server/src/test/java/org/apache/ambari/server/utils/StageUtilsTest.java 2d663d9

> 
> Diff: https://reviews.apache.org/r/43621/diff/
> 
> 
> Testing
> -------
> 
> Manual testing:
> 
> Created a cluster of 3 nodes waited 30 mins with UI running and collected postgres sql
statement stats, than left the cluster running for another 30 mins wiht UI closed and getting
new sql statement stats. Verified that the number of execution of  queries mentioned in the
JIRA has decreased.
> 
> Unit tests:
> 
> Total run:884
> Total errors:0
> Total failures:0
> OK
> 
> 
> Thanks,
> 
> Sebastian Toader
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message