> On Sept. 30, 2016, 12:39 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClustersImpl.java, > > lines 1104-1110 > > <https://reviews.apache.org/r/52425/diff/1/?file=1516872#file1516872line1104> > > > > 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.
This will use the ClusterFactory to essentially replace the cluster which now has outdated information in the Clusters internal collections. If the ClusterImpl is in use, no state data is being changed in it in this method; it will just return stale information (as it would have already done if it was serialized out to a web page). With your idea, we'd need to lock around the "refresh" of the internal data - that's what we wanted to get away from. > On Sept. 30, 2016, 12:39 p.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/utils/RetryHelper.java, > > lines 92-94 > > <https://reviews.apache.org/r/52425/diff/1/?file=1516873#file1516873line92> > > > > Aren't the affectedClusters in this collection left with potentially > > invalid data since they reference the old cluster instance? They are - they will be removed the next time a call comes in through the API though since the cluster will match on the name in the map. So I didn't think it was necessary to remove them. I could re-request them from Clusters in the case of invalidation. I really dislike this class. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52425/#review151033 ----------------------------------------------------------- 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 > >
