> On Feb. 18, 2016, 8: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.
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? > On Feb. 18, 2016, 8:27 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java, > > lines 526-539 > > <https://reviews.apache.org/r/43621/diff/2/?file=1254367#file1254367line526> > > > > Are there other places where you need to invalidate your cache? Such as > > updateStatusByRequestId? > > > > The problem with caches like this is that you could miss a place to > > invalidate them and you're perpetually stuck with stale values. > > Jonathan Hurley wrote: > Perhaps you also need a thread of some sort to periodically wipe the > cache just to ensure you don't miss places where you might not be > invalidating. The updateStatusByRequestId calls merge which merge(HostRoleCommandEntity) that calls invalidateHostRoleCommandStatusCache. Great idea to periodically wipe out the cache. Guava cache has support for cache expiry so we don't have to implement here anything compicated. - Sebastian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43621/#review119688 ----------------------------------------------------------- On Feb. 19, 2016, 3:35 p.m., Sebastian Toader wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43621/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2016, 3:35 p.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 > >
