Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of koushik.das@citrix.com
 designates 203.166.19.134 as permitted sender)
From: Koushik Das <koushik.das@citrix.com>
To: Frank Zhang <Frank.Zhang@citrix.com>, CloudStack DeveloperList
	<cloudstack-dev@incubator.apache.org>, 'Chiradeep Vittal'
	<chiradeepv@gmail.com>, Abhinandan Prateek <aprateek@apache.org>, Alex Huang
	<Alex.Huang@citrix.com>
Date: Thu, 10 Jan 2013 17:09:41 +0530
Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread pool
 size configurable
Thread-Topic: Review Request: CLOUDSTACK-810: Make DirectAgent thread pool
 size configurable
Thread-Index: Ac3uzvMfUX3wIIQ9Tc+WTLz5mrEH4QAALbUQABWzWkA=
Message-ID: 
 <2529883E7B666F4E8F21F85AADA43CA7010C8F39B563@BANPMAILBOX01.citrite.net>
References: 
 <93099572B72EB341B81A644E134F240B012F7342255A@SJCPMAILBOX01.citrite.net>
 <CD135428.4001B%chiradeep.vittal@citrix.com>
 <93099572B72EB341B81A644E134F240B012F7342255B@SJCPMAILBOX01.citrite.net>
In-Reply-To: 
 <93099572B72EB341B81A644E134F240B012F7342255B@SJCPMAILBOX01.citrite.net>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

So is it ok to leave it like this for now and revisit after Kelven's change=
s 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; Ale=
x
> Huang
> Cc: Koushik Das
> Subject: RE: Review Request: CLOUDSTACK-810: Make DirectAgent thread
> pool size configurable
>=20
> 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
>=20
> > -----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 (=A74.12.4).
> > >
> > >>T is a top level class (=A77.6), and an assert statement (=A714.10)
> > >>lexically nested within T (=A78.1.3) is executed.
> > >
> > >>A reference to a static field (=A78.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 initializati=
on.
> > >
> > >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
> > >> >
> > >> >
> > >