incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Huang <Alex.Hu...@citrix.com>
Subject RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread pool size configurable
Date Thu, 10 Jan 2013 13:25:35 GMT
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.

--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.Vittal@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.Zhang@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:noreply@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
> > > >> >
> > > >> >
> > > >


Mime
View raw message