I have updated the patch, can anyone do a review?
> -----Original Message-----
> From: Alex Huang
> Sent: Friday, January 11, 2013 8:43 PM
> To: Koushik Das; Frank Zhang; CloudStack DeveloperList; 'Chiradeep Vittal';
> Abhinandan Prateek
> Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread
> pool size configurable
>
> It can be created in configure() but cannot be started until start().
>
> --Alex
>
> > -----Original Message-----
> > From: Koushik Das
> > Sent: Friday, January 11, 2013 4:06 AM
> > To: Alex Huang; Frank Zhang; CloudStack DeveloperList; 'Chiradeep
> > Vittal'; Abhinandan Prateek
> > Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread
> > pool size configurable
> >
> > 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.jav
> > > > > > >> > a
> > > > > > 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
> > > > > > >> >
> > > > > > >> >
> > > > > > >
