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