----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52425/#review151033 -----------------------------------------------------------
Fix it, then Ship it! ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClustersImpl.java (lines 936 - 942) <https://reviews.apache.org/r/52425/#comment219160> If I invalidate a cluster instance for which some reference is in use, won't this cause issues unless the referenced invalidated cluser instance is refreshed? Would it be useful to tell the cluster instance that it has been invalidated so that when calls are issued on it, it will throw an exception. This might be a good debugging tool. ambari-server/src/main/java/org/apache/ambari/server/utils/RetryHelper.java (lines 87 - 89) <https://reviews.apache.org/r/52425/#comment219158> Aren't the affectedClusters in this collection left with potentially invalid data since they reference the old cluster instance? - Robert Levas On Sept. 30, 2016, 10:53 a.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52425/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2016, 10:53 a.m.) > > > Review request for Ambari, Nate Cole, Robert Levas, and Robert Nettleton. > > > Bugs: AMBARI-18495 > https://issues.apache.org/jira/browse/AMBARI-18495 > > > Repository: ambari > > > Description > ------- > > Many of the business object implementations include needless locks which > simply add the overhead and contention in larger clusters. Some examples of > these are : > > - HostImpl > -- get/set DisksInfo() > -- get/set TotalMemBytes() > > - ServiceComponentHostImpl > -- get/set MaintenanceState() > -- get/set LastOpLastUpdateTime() > > These types of methods are found on other business classes as well, like > {{ClusterImpl}} and {{ServiceImpl}}. Additionally, methods like > {{convertToResponse()}} and {{debugDump()}} need not acquire locks since they > are used mostly for serialization of data to the web client where the data > will then immediately become stale anyway. > > The {{Clusters}} and {{Cluster}} business objects should have the following > work performed: > - Remove locking around areas where its no longer required > - Replace collections with thread-safe concurrent versions > - Remove some reliance on state-full business objects (caches) > > *In general, the only locks which were left inside of ClusterImpl deal with > configurations and/or desired configs which are modeled in such a way that it > would require extensive changes to the DDL to eliminate the locks* > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 5e498f0 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java > d37e32b > ambari-server/src/main/java/org/apache/ambari/server/state/Cluster.java > d141df8 > ambari-server/src/main/java/org/apache/ambari/server/state/Clusters.java > 2d859b3 > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > a6f0a3b > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClustersImpl.java > 6318545 > ambari-server/src/main/java/org/apache/ambari/server/utils/RetryHelper.java > 877e84d > > ambari-server/src/test/java/org/apache/ambari/server/api/services/ClusterServiceTest.java > c36f5fe > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterResourceProviderTest.java > c7261ea > > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterDeadlockTest.java > ec01b80 > > Diff: https://reviews.apache.org/r/52425/diff/ > > > Testing > ------- > > PENDING > > > Thanks, > > Jonathan Hurley > >
