> On Jan. 4, 2016, 6:08 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java, > > line 373 > > <https://reviews.apache.org/r/41774/diff/4/?file=1180186#file1180186line373> > > > > Any reason we're using a Gson instance from another class here? Seems > > like we can just inject it directly into the ResourceProvider > > Dmitro Lisnichenko wrote: > This method is used 60+ times over different code. There is no docs in > utility class regarding that, but I assume that it is done either to avoid > frequent creation of Gson instances (code was committed due to perf jira > AMBARI-1539), or to be able to manage json generation parameters in one place. > > Jonathan Hurley wrote: > Seems wrong. GSON is bound as a singleton via Guice (from > ControllerModule) > `bind(Gson.class).in(Scopes.SINGLETON);` So that any injected GSON incurs > no initialization overhead.
You are right, but I was looking on source code of org.apache.ambari.server.utils.StageUtils#getGson It creates it's own instance: gson = new Gson(); I can change using gson instance from StageUtils to injection in my patch, or open a jira for a comprehensive fix - Dmitro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41774/#review112590 ----------------------------------------------------------- On Dec. 31, 2015, 5:47 p.m., Dmitro Lisnichenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41774/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2015, 5:47 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Dmytro Grinenko, Jonathan > Hurley, Jayush Luniya, Nate Cole, and Yusaku Sako. > > > Bugs: AMBARI-14520 > https://issues.apache.org/jira/browse/AMBARI-14520 > > > Repository: ambari > > > Description > ------- > > On 900 host cluster the call to install packages took like 3 minutes 20 > seconds which is kind of long. > {noformat} > POST http://perf-a-1:8080/api/v1/clusters/c1/stack_versions > > {"ClusterStackVersions":{"stack":"HDP","version":"2.2","repository_version":"2.2.9.0-3393"}}: > {noformat} > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariActionExecutionHelper.java > 6e7e06b > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java > b6ac291 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/HostStackVersionResourceProvider.java > 43a4423 > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > 3358e8c > > ambari-server/src/main/java/org/apache/ambari/server/state/host/HostImpl.java > c56d7ce > > Diff: https://reviews.apache.org/r/41774/diff/ > > > Testing > ------- > > mvn clean test > > Current request execution time: 0m22.678s > > With current patch, most time (about 40%) is consumed by > findConfigurationTagsWithOverrides(cluster, hostname), but it seems to be > inevitable even with batching addExecutionCommandsToStage() calls into a > single invocation. I've tried inserting logging statements to verify that > number of invocations equals to number of hosts. > Rest of time is mostly consumed by > org.apache.ambari.server.actionmanager.Stage.addHostRoleExecutionCommand() > (20% of time) and by N invocations of ClustersImpl.getHostsForCluster() (19% > of time). The latter calls can not be cached in method, so I can only rewrite > method signature > AmbariActionExecutionHelper.addExecutionCommandsToStage(ActionExecutionContext, > Stage) and cache it in ClusterStackVersionResourceProvider. Seems to be a > bad deal considering 1-3 seconds (on large clusters ) of performance boost > for a rare call vs readability/maintainability of code. > > > Thanks, > > Dmitro Lisnichenko > >
