Hi Marina,
The current behavior is a little different than I thought initially.
1. If the ORM file cannot be found. We log a Warning. In previous
discussions about other things, we have agreed that a WARNING is fairly
severe and that customers seeing warnings should probably correct them.
2. If there are errors in the ORM file, the toplink.orm.throw.exceptions
property is used.
This will be the last time I argue sematics with you in this thread,
but both of these things are designed to be this way and therefore are
not bugs.
I am still ok with changing the default of this property and even ok
with adding the "ORM file not found" issue to the list of things this
property will cover.
In my opinion, we have 1 feature. The feature is to make any error in
an ORM XML file throw an exception rather than log a message. If you
want to treat these two things as 2 separate features, that is ok with me.
One more comment about release notes. I do not pretend to have any
say in what goes into the GlassFish release notes, but if I were writing
them, I would seriously consider noting any behavior that could change
whether someone application worked from release to release in those
release notes. I think this change fits that category.
-Tom
Marina Vatkina wrote:
>Hi Tom,
>
>With the current bug (it is a bug) even if you try to turn exceptions on,
>the code will never reach that block. When Sahoo fixes the bug, the new
>feature will enable an exception by default.
>
>I agree with #1 and #3.
>
>thanks,
>-marina
>
>Tom Ware wrote:
>
>
>>Hi Marina,
>>
>> For some reason, I can't get to the GF web pages right now. I'll try
>>again later.
>>
>> Let's do #1 and #3
>>
>> I have started the process for #3 and it should be possible to complete
>>it by the time Sahoo finishes his changes.
>>
>> Are we ok with changing the current bug to a Feature request and
>>updating the subject line? Or do we want to close the current issue and
>>point to a new feature request?
>>
>>-Tom
>>
>>Marina Vatkina wrote:
>>
>>
>>
>>>Hi Tom,
>>>
>>>Fair enough. We have several options:
>>>
>>>1. File a feature request and mark it as fixed.
>>>2. Add a note to GF persistence page.
>>>3. Add a note to TopLink JPA page where you list this option.
>>>
>>>What would be your preference? I don't think it's a candidate for the
>>>release
>>>notes.
>>>
>>>thanks,
>>>-marina
>>>
>>>Tom Ware wrote:
>>>
>>>
>>>
>>>
>>>>Hi Marina,
>>>>
>>>>First of all, saying we are ignoring the issue is conpletely false.
>>>>A warning message is logged. In fact, having the exception thrown is
>>>>even a configuration option.
>>>>
>>>>We have now chosen to change this default value for this option.
>>>>
>>>>The fact is, that whether or not you personally like the initial
>>>>design choice, that is what the current product does and there could
>>>>be customers that are able to deploy an application that runs
>>>>successfully as a result of that choice. With this change, their
>>>>applications will potentially no longer deploy. I think it is
>>>>perfectly reasonable to publish this kind of information somewhere.
>>>>
>>>>My question is simply: In this open source environment, how do we
>>>>publish changes to defaults from build to build when they are NOT
>>>>bugs, but simply new design choices.
>>>>
>>>>-Tom
>>>>
>>>>
>>>>Marina Vatkina wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Tom,
>>>>>
>>>>>Are you concerned that throwing an exception when we should, instead
>>>>>of ignoring
>>>>>it will as we do today, will cause some users to clean up their
>>>>>applications?
>>>>>
>>>>>thanks,
>>>>>-marina
>>>>>
>>>>>Tom Ware wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Hi Sahoo,
>>>>>>
>>>>>>I am, in principal, ok with changing the default.
>>>>>>
>>>>>>Here is a more general question. If we decide to change a default
>>>>>>like this (that could affect backward compatibility for some
>>>>>>users). How do we notify them? To me, just closing the issue is
>>>>>>not really adequate in the case were we are making a conscious
>>>>>>choice to change behavior rather than fixing something that is
>>>>>>clearly a bug.
>>>>>>
>>>>>>-Tom
>>>>>>
>>>>>>Sanjeeb Kumar Sahoo wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>Hi Tom,
>>>>>>>
>>>>>>>Tom Ware wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>Hi Sahoo,
>>>>>>>>
>>>>>>>>Choosing to just warn for an orm.xml file that does not exist was
>>>>>>>>a design choice. I think both the current behavior and the
>>>>>>>>behavior you are suggesting are quite reasonable.
>>>>>>>>We initially chose to only send a warning to allow developers to
>>>>>>>>continue to work with their persistence unit even if there were
>>>>>>>>some errors in some of the files. Changing the value
>>>>>>>>toplink.orm.throw.exceptions should allow the exceptions to
>>>>>>>>actually be thrown. Is simply documenting this property adequate
>>>>>>>>to fix this issue?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>No. I think, we should change the default (see further explanation
>>>>>>>given below) in addition to documenting the property.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>Is there any particular reason why we are choosing to change this
>>>>>>>>default?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>More often than not, an application will run into some kind of
>>>>>>>trouble if a mapping file is not processed. The submitter of issue
>>>>>>>#1051 also wants us to throw exception. I support him, because we
>>>>>>>both wasted 15 to 20 minutes before we found out the typo in
>>>>>>>mapping file name. Given the number of messages that get logged,
>>>>>>>it is very easy to miss a warning. Why would a (real) developer
>>>>>>>ever want to specify a non-existent mapping file? So I think,
>>>>>>>throwing an exception is a more developer friendly choice.
>>>>>>>
>>>>>>>So, let me know if I can change the default behavior or not. I
>>>>>>>would also like to know what others think.
>>>>>>>
>>>>>>>Thanks,
>>>>>>>Sahoo
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>-Tom
>>>>>>>>
>>>>>>>>Sanjeeb Kumar Sahoo wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>Hi Tom,
>>>>>>>>>
>>>>>>>>>I am working on
>>>>>>>>>https://glassfish.dev.java.net/issues/show_bug.cgi?id=1051. The
>>>>>>>>>current behavior is to log a message when mapping file does not
>>>>>>>>>exist. I am planning to change it to throw an exception by
>>>>>>>>>default, but I see some entity-persistence-tests failing as they
>>>>>>>>>specifically test for the current behavior. The persistence.xml
>>>>>>>>>has such text:
>>>>>>>>><!-- this is here for testing purposes - ensures a persistence
>>>>>>>>>unit can load even with a file that does not exist. Please do
>>>>>>>>>not add an xml file called: META-INF/non-existant-orm-file.xml -->
>>>>>>>>>
>>>>>>>>><mapping-file>META-INF/non-existant-orm-file.xml</mapping-file>
>>>>>>>>>
>>>>>>>>>Is there a specific reason behind the current behavior? Can we
>>>>>>>>>change it?
>>>>>>>>>
>>>>>>>>>Also, what's this property called toplink.orm.throw.exceptions?
>>>>>>>>>I don't find any documentation for this property. It is used in
>>>>>>>>>current code.
>>>>>>>>>
>>>>>>>>>Thanks,
>>>>>>>>>Sahoo
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com