Hi Alex, Need some clarification based on your comments. See inline.
> -----Original Message----- > From: Alex Huang > Sent: Thursday, January 10, 2013 6:56 PM > To: Koushik Das; Frank Zhang; CloudStack DeveloperList; 'Chiradeep Vittal'; > Abhinandan Prateek > Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread > pool size configurable > > You should change it because Frank is wrong. Spring has the same problem. > No injection framework can override the static initializers. This "pain" is > pain > of java not designing dependency injection into the language spec in the first > place. Has nothing to do with ComponentLocator. > > ComponentLocator forces a specific life cycle in the components it manages. > That's why it specifies three types of interfaces. For the managers, no > thread > should start until the start method is called. That's the rule direct agent > violated. You should figure out how to start the direct agent threadpool > inside the start() method of agentmanager. Should the thread pool be created in the start() or the configure() method of the agent manager? I see that that other thread pools getting created in configure itself. This is what I am planning to do. Move the direct agent thread pool in AgentManagerImpl class. The DirectAgentAttache will access it via the AgentManagerImpl reference. > > --Alex > > > -----Original Message----- > > From: Koushik Das > > Sent: Thursday, January 10, 2013 3:40 AM > > To: Frank Zhang; CloudStack DeveloperList; 'Chiradeep Vittal'; > > Abhinandan Prateek; Alex Huang > > Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread > > pool size configurable > > > > So is it ok to leave it like this for now and revisit after Kelven's > > changes are > in. > > Or should I move it in AgentManagerImpl? > > > > -Koushik > > > > > -----Original Message----- > > > From: Frank Zhang > > > Sent: Thursday, January 10, 2013 6:46 AM > > > To: CloudStack DeveloperList; 'Chiradeep Vittal'; Abhinandan > > > Prateek; Alex Huang > > > Cc: Koushik Das > > > Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent > thread > > > pool size configurable > > > > > > Yeah I agree. > > > As Kelven is replacing injection with Spring, we are eased from this pain. > > > Spring supports init function which guarantees be called after all > > > dependencies of this bean being loaded. > > > Then we can inject configDao in AgentManagerImpl and initialize the > > > value > > in > > > a init(). > > > This avoid changing current componentloader to make ConfigDao load > > > before AgentManager > > > > > > > -----Original Message----- > > > > From: Chiradeep Vittal [mailto:chiradeep.vit...@citrix.com] > > > > Sent: Wednesday, January 09, 2013 5:08 PM > > > > To: CloudStack DeveloperList; 'Chiradeep Vittal'; Abhinandan > > > > Prateek; Alex Huang > > > > Cc: Koushik Das > > > > Subject: Re: Review Request: CLOUDSTACK-810: Make DirectAgent > > thread > > > > pool size configurable > > > > > > > > You may be right about the specific order of things in this > > > > particular case, but we shouldn't assume that the order of > > > > initialization is going to be the same forever. > > > > > > > > On 1/9/13 4:59 PM, "Frank Zhang" <frank.zh...@citrix.com> wrote: > > > > > > > > >Though I agree we can put it in initialization code of > > > > >AgentManagerImpl, we have to guarantee AgentManagerImpl is > > loaded > > > > before ConfigDao. > > > > >Technically I think it's ok to the Dao is guaranteed before this > > > > >static block of DirectoAgentAttache. > > > > > > > > > >A static block is invoked when a class type initialized, and from > > > > >Java spec, a class type is initialized when > > > > > > > > > >>12.4.1. When Initialization Occurs > > > > > > > > > >>A class or interface type T will be initialized immediately > > > > >>before the first occurrence of any one of the following: > > > > > > > > > >>T is a class and an instance of T is created. > > > > > > > > > >>T is a class and a static method declared by T is invoked. > > > > > > > > > >>A static field declared by T is assigned. > > > > > > > > > >>A static field declared by T is used and the field is not a > > > > >>constant variable (§4.12.4). > > > > > > > > > >>T is a top level class (§7.6), and an assert statement (§14.10) > > > > >>lexically nested within T (§8.1.3) is executed. > > > > > > > > > >>A reference to a static field (§8.3.1.1) causes initialization > > > > >>of only the class or interface that actually declares it, even > > > > >>though it might be referred to through the name of a subclass, a > > > > >>subinterface, or a class that implements an interface. > > > > > > > > > >>Invocation of certain reflective methods in class Class and in > > > > >>package java.lang.reflect also causes class or interface > > > > >>initialization. > > > > > > > > > >For Dao is loaded when componentloader initialized, at that time > > > > >being we don't have code path pertaining to DirectAgentAttach class. > > > > > > > > > > > > > > > > > > > > > > > > >> -----Original Message----- > > > > >> From: Chiradeep Vittal [mailto:nore...@reviews.apache.org] On > > > > >> Behalf Of Chiradeep Vittal > > > > >> Sent: Wednesday, January 09, 2013 4:39 PM > > > > >> To: Abhinandan Prateek; Alex Huang > > > > >> Cc: cloudstack; Koushik Das; Chiradeep Vittal > > > > >> Subject: Re: Review Request: CLOUDSTACK-810: Make DirectAgent > > > > thread > > > > >> pool size configurable > > > > >> > > > > >> > > > > >> ----------------------------------------------------------- > > > > >> This is an automatically generated e-mail. To reply, visit: > > > > >> https://reviews.apache.org/r/8855/#review15219 > > > > >> ----------------------------------------------------------- > > > > >> > > > > >> > > > > >> > > > > >> server/src/com/cloud/agent/manager/DirectAgentAttache.java > > > > >> <https://reviews.apache.org/r/8855/#comment32827> > > > > >> > > > > >> We cannot assume that the Daos will be loaded before this > > > > >>static initializer is called. The order of calling static > > > > >>initializers is not defined. It is better to initialize it via > > > > >>the AgentManagerImpl > > > > >> > > > > >> > > > > >> - Chiradeep Vittal > > > > >> > > > > >> > > > > >> On Jan. 8, 2013, 8:06 a.m., Koushik Das wrote: > > > > >> > > > > > >> > ----------------------------------------------------------- > > > > >> > This is an automatically generated e-mail. To reply, visit: > > > > >> > https://reviews.apache.org/r/8855/ > > > > >> > ----------------------------------------------------------- > > > > >> > > > > > >> > (Updated Jan. 8, 2013, 8:06 a.m.) > > > > >> > > > > > >> > > > > > >> > Review request for cloudstack, Abhinandan Prateek and Alex > Huang. > > > > >> > > > > > >> > > > > > >> > Description > > > > >> > ------- > > > > >> > > > > > >> > Currently the DirectAgent pool size is hard-coded to 500. One > > > > >> > of the > > > > >>factors > > > > >> that can affect this is the number of hosts in a deployment. If > > > > >>there are more than 500 hosts (say around 1K) then this pool > > > > >>can easily get exhausted resulting in delays and undesired behavior. > > > > >> > > > > > >> > Removed hard-coding of directagent thread pool size and now > > > > >> > reading it > > > > >> from configuration. > > > > >> > > > > > >> > > > > > >> > This addresses bug CLOUDSTACK-810. > > > > >> > > > > > >> > > > > > >> > Diffs > > > > >> > ----- > > > > >> > > > > > >> > server/src/com/cloud/agent/manager/DirectAgentAttache.java > > > > 848c7e6 > > > > >> > server/src/com/cloud/configuration/Config.java 92313ea > > > > >> > setup/db/db/schema-40to410.sql 4455956 > > > > >> > > > > > >> > Diff: https://reviews.apache.org/r/8855/diff/ > > > > >> > > > > > >> > > > > > >> > Testing > > > > >> > ------- > > > > >> > > > > > >> > Verified that configuration entry is present in the 'configuration' > > > > >>table. > > > > >> > Also verified in a debugger that it is read correctly while > > > > >> > creating > > > > >>the > > > > >> directagent thread pool. > > > > >> > > > > > >> > > > > > >> > Thanks, > > > > >> > > > > > >> > Koushik Das > > > > >> > > > > > >> > > > > > >
