Re: Don't give me static...

[Justin Harris]

----- "Jesus M. Rodriguez" <jesusr(a)redhat.com&gt; wrote:

>  -----BEGIN PGP SIGNED MESSAGE-----
>  Hash: SHA1
>  
>  On 06/01/2010 04:16 PM, Justin Harris wrote:
>  > I was just burned pretty bad by a subtle bug and want to share some
>  > general thoughts with folks.  The core of the issue that I had was
>  that
>  > the Config class (which is not static) held a static map that
>  represents
>  > the internal state of the object.  Additionally, this static map
>  was
>  > only initialized upon the first creation of a Config object.  This
>  > doesn't really cause much harm in a deployed environment, but it
>  caused
>  > a lot of pain in our unit test suite -- specifically why one of the
>  > tests would pass when run by itself, and yet failed when run in the
>  > larger test suite.
>  
>  But why is this the case? is it that we are changing the config in
>  one
>  test? If a test changes the config, they should change it BACK. 
Why have to worry about all this state juggling, when each test can 
start with a blank slate and create what it needs?  I agree that there
are situations where that might not be as easy to do, or more expensive
than is tolerable, but this is not one of those cases.

>  
>  > So I think that the lesson learned is:  Don't use static anything. 
>  Or,
>  
>  I wouldn't go as far as saying "don't use static anything", I still
>  think
>  there are some uses for it. Like all code we just need to be careful
>  how we use things. Using static doesn't necessarily make the code bad
>  just like not using static doesn't make the code good. 
I agree with this, but it does introduce unnecessary coupling between components,
one of the very practices that frameworks such as Guice are aiming to overcome.  You make
a good point in the bootstrap process, as getting Guice up and running may require us
to make use of static classes, but I do think that as much as possible, we should inject
dependencies and let Guice handle tying them together.

>  
>  > at the very least, treat static classes/variables as a code smell,
>  and
>  > really try to justify it before throwing it in.  One of the really
>  nice
>  > aspects of Guice is its ability to abstract away how the
>  dependencies of
>  > a component are created, and I think we should defer to the
>  framework
>  > for e.g. creating objects as singletons for optimization purposes.
>  
>  Since we are using Guice, it does make sense that we would use it to
>  create a singleton of the Config, but how does that affect the custom
>  modules we load in the Config to configure Guice? Is this a chicken
>  and the egg problem? 
Quite possibly, maybe we are overloading the Config class too much?  One possible
solution is to split the config into:

1) Config to get Guice up and running
2) Config component that can be injected into other objects

>  
>  > What do you guys think?  Am I overreacting?  :)
>  
>  See above, and static's aren't always bad.
>  
>  - -- 
>  jesus m. rodriguez          | jesusr(a)redhat.com
>  principal software engineer | irc: zeus
>  red hat systems management  | 919.754.4413 (w)
>  rhce # 805008586930012      | 919.623.0080 (c)
>  +---------------------------------------------+
>  |   "Those who cannot remember the past       |
>  |    are condemned to repeat it."             |
>  |                        -- George Santayana  |
>  +---------------------------------------------+
>  
> >  -----BEGIN PGP SIGNATURE-----
> >  Version: GnuPG v1.4.9 (GNU/Linux)
> >  Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> >  
> >  iEYEARECAAYFAkwFb6YACgkQvJZ57YntiYN3GwCfbvv1qQsj9wQv7LJgCC4STvhJ
> >  /poAnip3xxAGjvbg09zzEwgZVYi/MlpH
> >  =H/Uy
> >  -----END PGP SIGNATURE----- 
>  _______________________________________________
>  candlepin mailing list
>  candlepin(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/candlepin

Don't give me static...

[Adam Young]

I agree with Justin on this.  There is a difference between a Class and 
instance.  People often create somethiing as a singleton because they 
want to have a known way to access an instance.

Dependecy injection works on a separation of the policy used to create 
an object from the object itsef.  When you use the word "static"  you 
are tying yourself to a creation policy.  This causes a whole slew of 
problems that boil down to "I only want one little thing, and I end up 
pulling in the whole kitchen sink."


When you call on an object you should think about:
What is the contract that this object is bound to.


When you implement an object you should think about:
What is the object is supposed to do?


In neither case should you care about:
How is this object created.


Object lifespans are not really related to that discussion.  Objects 
tend to live and die in hierarchical groups.  Everything in an http 
request can be freed at once.  Everything in a session can be freed 
together, along with all of the requests that hand off of them.  Static 
implies a lifesspan longer than most web containers support.  If you 
want something that is global to your application, tell the container 
and it will manage that for you.







On 06/01/2010 04:55 PM, Justin Harris wrote:
>  ----- "Jesus M. Rodriguez"<jesusr(a)redhat.com> 
> wrote:
>
>     
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 06/01/2010 04:16 PM, Justin Harris wrote:
> >      
> >> I was just burned pretty bad by a subtle bug and want to share some
> >> general thoughts with folks.  The core of the issue that I had was
> >>        
> > that
> >      
> >> the Config class (which is not static) held a static map that
> >>        
> > represents
> >      
> >> the internal state of the object.  Additionally, this static map
> >>        
> > was
> >      
> >> only initialized upon the first creation of a Config object.  This
> >> doesn't really cause much harm in a deployed environment, but it
> >>        
> > caused
> >      
> >> a lot of pain in our unit test suite -- specifically why one of the
> >> tests would pass when run by itself, and yet failed when run in the
> >> larger test suite.
> >>        
> > But why is this the case? is it that we are changing the config in
> > one
> > test? If a test changes the config, they should change it BACK.
> >      
>  Why have to worry about all this state juggling, when each test can
>  start with a blank slate and create what it needs?  I agree that there
>  are situations where that might not be as easy to do, or more expensive
>  than is tolerable, but this is not one of those cases.
>
>     
> >      
> >> So I think that the lesson learned is:  Don't use static anything.
> >>        
> > Or,
> >
> > I wouldn't go as far as saying "don't use static anything", I
> still
> > think
> > there are some uses for it. Like all code we just need to be careful
> > how we use things. Using static doesn't necessarily make the code bad
> > just like not using static doesn't make the code good.
> >      
>  I agree with this, but it does introduce unnecessary coupling between components,
>  one of the very practices that frameworks such as Guice are aiming to overcome.  You
> make
>  a good point in the bootstrap process, as getting Guice up and running may require us
>  to make use of static classes, but I do think that as much as possible, we should inject
>  dependencies and let Guice handle tying them together.
>
>     
> >      
> >> at the very least, treat static classes/variables as a code smell,
> >>        
> > and
> >      
> >> really try to justify it before throwing it in.  One of the really
> >>        
> > nice
> >      
> >> aspects of Guice is its ability to abstract away how the
> >>        
> > dependencies of
> >      
> >> a component are created, and I think we should defer to the
> >>        
> > framework
> >      
> >> for e.g. creating objects as singletons for optimization purposes.
> >>        
> > Since we are using Guice, it does make sense that we would use it to
> > create a singleton of the Config, but how does that affect the custom
> > modules we load in the Config to configure Guice? Is this a chicken
> > and the egg problem?
> >      
>  Quite possibly, maybe we are overloading the Config class too much?  One possible
>  solution is to split the config into:
>
>  1) Config to get Guice up and running
>  2) Config component that can be injected into other objects
>
>     
> >      
> >> What do you guys think?  Am I overreacting?  :)
> >>        
> > See above, and static's aren't always bad.
> >
> > - -- 
> > jesus m. rodriguez          | jesusr(a)redhat.com
> > principal software engineer | irc: zeus
> > red hat systems management  | 919.754.4413 (w)
> > rhce # 805008586930012      | 919.623.0080 (c)
> > +---------------------------------------------+
> > |   "Those who cannot remember the past       |
> > |    are condemned to repeat it."             |
> > |                        -- George Santayana  |
> > +---------------------------------------------+
> >
> > > -----BEGIN PGP SIGNATURE-----
> > > Version: GnuPG v1.4.9 (GNU/Linux)
> > > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> > >
> > > iEYEARECAAYFAkwFb6YACgkQvJZ57YntiYN3GwCfbvv1qQsj9wQv7LJgCC4STvhJ
> > > /poAnip3xxAGjvbg09zzEwgZVYi/MlpH
> > > =H/Uy
> > > -----END PGP SIGNATURE----- 
> >
> _______________________________________________
> > candlepin mailing list
> > candlepin(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/candlepin
> >      
>  _______________________________________________
>  candlepin mailing list
>  candlepin(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/candlepin
>

Don't give me static...

[James Bowes]

On Tue, Jun 01, 2010 at 04:55:11PM -0400, Justin Harris wrote:
<snip> 
>  > Since we are using Guice, it does make sense that we would use
> it to
>  > create a singleton of the Config, but how does that affect the custom
>  > modules we load in the Config to configure Guice? Is this a chicken
>  > and the egg problem?
>  
>  Quite possibly, maybe we are overloading the Config class too much?  One possible
>  solution is to split the config into:
>  
>  1) Config to get Guice up and running
>  2) Config component that can be injected into other objects 
I'd prefer to just instantiate the config twice rather than complicate
things to work around something that only happens once.

Or, we go whole hog and make classes for each configurable part of
candlepin.

>  > 
>  > > What do you guys think?  Am I overreacting?  :)
>  > 
>  > See above, and static's aren't always bad. 
new Config() sticks out with all the other guiceyness, so I'm all for
changing that. I think static makes sense for a config map, however, as
in the non-testing code, you wouldn't want to be altering any of those
values.

-James

Don't give me static...

[Adam Young]

On 06/02/2010 06:58 AM, James Bowes wrote:
>  On Tue, Jun 01, 2010 at 04:55:11PM -0400, Justin Harris wrote:
>  <snip>
>     
> >> Since we are using Guice, it does make sense that we would use it to
> >> create a singleton of the Config, but how does that affect the custom
> >> modules we load in the Config to configure Guice? Is this a chicken
> >> and the egg problem?
> >>        
> > Quite possibly, maybe we are overloading the Config class too much?  One possible
> > solution is to split the config into:
> >
> > 1) Config to get Guice up and running
> > 2) Config component that can be injected into other objects
> >      
>  I'd prefer to just instantiate the config twice rather than complicate
>  things to work around something that only happens once.
>
>  Or, we go whole hog and make classes for each configurable part of
>  candlepin.
>
>     
> >>        
> >>> What do you guys think?  Am I overreacting?  :)
> >>>          
> >> See above, and static's aren't always bad.
> >>        
>  new Config() sticks out with all the other guiceyness, so I'm all for
>  changing that. I think static makes sense for a config map, however, as
>  in the non-testing code, you wouldn't want to be altering any of those
>  values.
>      
   Typically, in a unix Daemon, kill -1 means reload your config.  So, 
you would want to reload at that point, possibly by just creating a new 
instance.
>  -James
>     
>
>
>  _______________________________________________
>  candlepin mailing list
>  candlepin(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/candlepin
>