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 Fri, 11 Jan 2013 15:12:57 GMT
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.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