hq r1390 - in trunk: sql/events src/org/hyperic/hibernate src/org/hyperic/hq src/org/hyperic/hq/bizapp/server/action/email src/org/hyperic/hq/bizapp/server/action/log src/org/hyperic/hq/events src/org/hyperic/hq/events/escalation src/org/hype

[jtravis_hyperic]

A couple things, inline:


On Nov 18, 2006, at 7:23 AM, youngl@hyperic.com wrote:

> Author: youngl
> Date: 2006-11-18 07:23:31 -0800 (Sat, 18 Nov 2006)
> New Revision: 1390
> URL: http://svn.hyperic.org/?view=rev&root=Hyperic+HQ&revision=1390
>

[snip]
> + * Find an alert pojo by ID
> + *
> + * @ejb:interface-method
> + */
> + public Alert getAlertById(Integer id) {
> + Alert alert = getAlertDAO().findById(id);
> + Hibernate.initialize(alert);
> + return alert;
> + }

2 things here. Why are you calling Hibernate.initialize()? This
should not be necessary and is definitely going to decrease
performance. And why are you calling findById from getAlertById?
the find() vs get() semantics should be different (find should throw
an exception.)

For the uninitiated: Hibernate.initialize() initializes a Hibernate
proxy or a collection. Lazy properties or collections which haven't
been read from the database will automatically be read -- basically
'unlazifying' the objects.

-- Jon

[admin]

The API does what the caller expects. The caller expects the condition
logs be avaialble and the caller may not
be in Hibernate session context. findbyid is exactly what is needed as
well for this use case.

- Young

Jon Travis wrote:
> A couple things, inline:
>
>
> On Nov 18, 2006, at 7:23 AM, youngl@hyperic.com wrote:
>
>> Author: youngl
>> Date: 2006-11-18 07:23:31 -0800 (Sat, 18 Nov 2006)
>> New Revision: 1390
>> URL: http://svn.hyperic.org/?view=rev&root=Hyperic+HQ&revision=1390
>>
>
> [snip]
>> + * Find an alert pojo by ID
>> + *
>> + * @ejb:interface-method
>> + */
>> + public Alert getAlertById(Integer id) {
>> + Alert alert = getAlertDAO().findById(id);
>> + Hibernate.initialize(alert);
>> + return alert;
>> + }
>
> 2 things here. Why are you calling Hibernate.initialize()? This
> should not be necessary and is definitely going to decrease
> performance. And why are you calling findById from getAlertById? the
> find() vs get() semantics should be different (find should throw an
> exception.)
>
> For the uninitiated: Hibernate.initialize() initializes a Hibernate
> proxy or a collection. Lazy properties or collections which haven't
> been read from the database will automatically be read -- basically
> 'unlazifying' the objects.
>
> -- Jon
>
>

[jtravis_hyperic]

So, anytime I need to find an alert by ID, it's going to initialize
the data from the DB, despite the fact that I may not use it?

That is inconsistent with the entire rest of our codebase. Hibernate
gives us the ability to use lazily loaded objects ... adding changes
like this eliminates that possibility.

Nearly every method we have in our system should expect to be run
within a transaction and session.

As for findById(). Do you want the method to throw an exception or
not? get() means it should return null, find() means it should throw
an exception. We have legacy code which does something different,
but we are trying to become consistent. Please read our Wiki entry
on the matter.

-- Jon



On Nov 20, 2006, at 11:56 AM, Young Lee wrote:

> The API does what the caller expects. The caller expects the
> condition logs be avaialble and the caller may not
> be in Hibernate session context. findbyid is exactly what is
> needed as well for this use case.
>
> - Young
>
> Jon Travis wrote:
>> A couple things, inline:
>>
>>
>> On Nov 18, 2006, at 7:23 AM, youngl@hyperic.com wrote:
>>
>>> Author: youngl
>>> Date: 2006-11-18 07:23:31 -0800 (Sat, 18 Nov 2006)
>>> New Revision: 1390
>>> URL: http://svn.hyperic.org/?view=rev&root=Hyperic+HQ&revision=1390
>>>
>>
>> [snip]
>>> + * Find an alert pojo by ID
>>> + *
>>> + * @ejb:interface-method
>>> + */
>>> + public Alert getAlertById(Integer id) {
>>> + Alert alert = getAlertDAO().findById(id);
>>> + Hibernate.initialize(alert);
>>> + return alert;
>>> + }
>>
>> 2 things here. Why are you calling Hibernate.initialize()? This
>> should not be necessary and is definitely going to decrease
>> performance. And why are you calling findById from getAlertById?
>> the find() vs get() semantics should be different (find should
>> throw an exception.)
>>
>> For the uninitiated: Hibernate.initialize() initializes a
>> Hibernate proxy or a collection. Lazy properties or collections
>> which haven't been read from the database will automatically be
>> read -- basically 'unlazifying' the objects.
>>
>> -- Jon
>>
>>

[admin]

It seems that there is a confusion over our naming convention.

For the Hibernate DAOs, we are following a convention where findXXX()
should throw an ObjectNotFoundException, and getXXX() returns null if
it can't find the object. There is a mix usage in this function that
should be made consistent. Hibernate's Session.get() does not throw
an exception if the data is not present, so we follow that. However,
we incorrectly state in our wiki docs that we are following the
Hibernate convention for finds. The reason why we expect findXXX()
to throw an exception is because our HibernateDAO's findById() does a
load(), which would throw an ObjectNotFoundException. I think we
should absolutely have a convention in naming our methods, but I'm
not sure if we have correctly stated it in our wiki docs. We have
plenty of other findXXX() methods that are expected to return
collections, and will definitely return a collection of zero size if
none is found without throwing an exception. So, it seems that get/
find will only apply to single object lookups. Furthermore, we also
haven't agreed on how we should declare runtime exceptions in these
cases. So let's decide whether or not we want to declare them on the
finders.

In regards to calling Hibernate.initialize(), I think the intent here
is to load all of the values. I am guessing that it's because in the
use case the pojo would get disassociated from the transaction? I
suppose that a more clear method name would make sure that other
consumers of the DAO would know what this method does, because
otherwise we will end up with a mixture of functions that all look
like findXXXById() and some will be lazily loaded, and some not. Can
you force the load by getting the caller (I assume it to be a
manager) to either use the value object from getAlertValue() on the
pojo or initializing the pojo after looking it up? I think as it
stands, it seems a bit confusing.

Charles



On Nov 20, 2006, at 12:03 PM, Jon Travis wrote:

> So, anytime I need to find an alert by ID, it's going to initialize
> the data from the DB, despite the fact that I may not use it?
>
> That is inconsistent with the entire rest of our codebase.
> Hibernate gives us the ability to use lazily loaded objects ...
> adding changes like this eliminates that possibility.
>
> Nearly every method we have in our system should expect to be run
> within a transaction and session.
>
> As for findById(). Do you want the method to throw an exception or
> not? get() means it should return null, find() means it should
> throw an exception. We have legacy code which does something
> different, but we are trying to become consistent. Please read our
> Wiki entry on the matter.
>
> -- Jon
>
>
>
> On Nov 20, 2006, at 11:56 AM, Young Lee wrote:
>
>> The API does what the caller expects. The caller expects the
>> condition logs be avaialble and the caller may not
>> be in Hibernate session context. findbyid is exactly what is
>> needed as well for this use case.
>>
>> - Young
>>
>> Jon Travis wrote:
>>> A couple things, inline:
>>>
>>>
>>> On Nov 18, 2006, at 7:23 AM, youngl@hyperic.com wrote:
>>>
>>>> Author: youngl
>>>> Date: 2006-11-18 07:23:31 -0800 (Sat, 18 Nov 2006)
>>>> New Revision: 1390
>>>> URL: http://svn.hyperic.org/?view=rev&root=Hyperic+HQ&revision=1390
>>>>
>>>
>>> [snip]
>>>> + * Find an alert pojo by ID
>>>> + *
>>>> + * @ejb:interface-method
>>>> + */
>>>> + public Alert getAlertById(Integer id) {
>>>> + Alert alert = getAlertDAO().findById(id);
>>>> + Hibernate.initialize(alert);
>>>> + return alert;
>>>> + }
>>>
>>> 2 things here. Why are you calling Hibernate.initialize()? This
>>> should not be necessary and is definitely going to decrease
>>> performance. And why are you calling findById from
>>> getAlertById? the find() vs get() semantics should be different
>>> (find should throw an exception.)
>>>
>>> For the uninitiated: Hibernate.initialize() initializes a
>>> Hibernate proxy or a collection. Lazy properties or collections
>>> which haven't been read from the database will automatically be
>>> read -- basically 'unlazifying' the objects.
>>>
>>> -- Jon
>>>
>>>
>

[jtravis_hyperic]

Glad the get/find was cleared up. We could probably augment the
documentation to contain details on collections.

We can open a separate discussion on runtime exceptions if you want
to talk about it.

As for Hibernate.initialize(). Right now those findById()s are doing
potentially unneeded database lookups. That is one of the problems I
have with disassociating/reassociating objects with the session -- it
removes Hibernate's ability to lazily load associations when the UI
is rendering.

I'm in support of the Open Session In View pattern as on the
HIbernate site -- that's what I've used in the past. By opening a
session in a filter, it allows us to use the same APIs everywhere
(tests, UI, etc.) and get the benefit of lazily initialized objects.
(i.e. we should not need a findInitializedById() and a findById())

-- Jon



On Nov 20, 2006, at 2:57 PM, Charles Lee wrote:

> It seems that there is a confusion over our naming convention.
>
> For the Hibernate DAOs, we are following a convention where findXXX
> () should throw an ObjectNotFoundException, and getXXX() returns
> null if it can't find the object. There is a mix usage in this
> function that should be made consistent. Hibernate's Session.get()
> does not throw an exception if the data is not present, so we
> follow that. However, we incorrectly state in our wiki docs that
> we are following the Hibernate convention for finds. The reason
> why we expect findXXX() to throw an exception is because our
> HibernateDAO's findById() does a load(), which would throw an
> ObjectNotFoundException. I think we should absolutely have a
> convention in naming our methods, but I'm not sure if we have
> correctly stated it in our wiki docs. We have plenty of other
> findXXX() methods that are expected to return collections, and will
> definitely return a collection of zero size if none is found
> without throwing an exception. So, it seems that get/find will
> only apply to single object lookups. Furthermore, we also haven't
> agreed on how we should declare runtime exceptions in these cases.
> So let's decide whether or not we want to declare them on the finders.
>
> In regards to calling Hibernate.initialize(), I think the intent
> here is to load all of the values. I am guessing that it's because
> in the use case the pojo would get disassociated from the
> transaction? I suppose that a more clear method name would make
> sure that other consumers of the DAO would know what this method
> does, because otherwise we will end up with a mixture of functions
> that all look like findXXXById() and some will be lazily loaded,
> and some not. Can you force the load by getting the caller (I
> assume it to be a manager) to either use the value object from
> getAlertValue() on the pojo or initializing the pojo after looking
> it up? I think as it stands, it seems a bit confusing.
>
> Charles
>
>
>
> On Nov 20, 2006, at 12:03 PM, Jon Travis wrote:
>
>> So, anytime I need to find an alert by ID, it's going to
>> initialize the data from the DB, despite the fact that I may not
>> use it?
>>
>> That is inconsistent with the entire rest of our codebase.
>> Hibernate gives us the ability to use lazily loaded objects ...
>> adding changes like this eliminates that possibility.
>>
>> Nearly every method we have in our system should expect to be run
>> within a transaction and session.
>>
>> As for findById(). Do you want the method to throw an exception
>> or not? get() means it should return null, find() means it should
>> throw an exception. We have legacy code which does something
>> different, but we are trying to become consistent. Please read
>> our Wiki entry on the matter.
>>
>> -- Jon
>>
>>
>>
>> On Nov 20, 2006, at 11:56 AM, Young Lee wrote:
>>
>>> The API does what the caller expects. The caller expects the
>>> condition logs be avaialble and the caller may not
>>> be in Hibernate session context. findbyid is exactly what is
>>> needed as well for this use case.
>>>
>>> - Young
>>>
>>> Jon Travis wrote:
>>>> A couple things, inline:
>>>>
>>>>
>>>> On Nov 18, 2006, at 7:23 AM, youngl@hyperic.com wrote:
>>>>
>>>>> Author: youngl
>>>>> Date: 2006-11-18 07:23:31 -0800 (Sat, 18 Nov 2006)
>>>>> New Revision: 1390
>>>>> URL: http://svn.hyperic.org/?view=rev&root=Hyperic
>>>>> +HQ&revision=1390
>>>>>
>>>>
>>>> [snip]
>>>>> + * Find an alert pojo by ID
>>>>> + *
>>>>> + * @ejb:interface-method
>>>>> + */
>>>>> + public Alert getAlertById(Integer id) {
>>>>> + Alert alert = getAlertDAO().findById(id);
>>>>> + Hibernate.initialize(alert);
>>>>> + return alert;
>>>>> + }
>>>>
>>>> 2 things here. Why are you calling Hibernate.initialize()?
>>>> This should not be necessary and is definitely going to decrease
>>>> performance. And why are you calling findById from
>>>> getAlertById? the find() vs get() semantics should be different
>>>> (find should throw an exception.)
>>>>
>>>> For the uninitiated: Hibernate.initialize() initializes a
>>>> Hibernate proxy or a collection. Lazy properties or collections
>>>> which haven't been read from the database will automatically be
>>>> read -- basically 'unlazifying' the objects.
>>>>
>>>> -- Jon
>>>>
>>>>
>>
>