You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Kevin Sutter (JIRA)" <ji...@apache.org> on 2009/12/02 01:16:20 UTC
[jira] Commented: (OPENJPA-1403) OSGi Aware Persistence Provider
Implementation
[ https://issues.apache.org/jira/browse/OPENJPA-1403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12784556#action_12784556 ]
Kevin Sutter commented on OPENJPA-1403:
---------------------------------------
Hi Milinda,
I haven't fully absorbed your patch and its implications yet, but I do have some initial comments on the patch content and format...
o Classloading is a touchy area. OpenJPA needs to work in a standalone Java SE environment, an application server Java EE environment, and now the OSGi bundled environment. As we integrated with the various application servers, we had to ensure that the classloading mechanisms continued to work in these other environments. I'm not sure how much verification you have done with your proposed patch in these other environments. On first glance, I didn't see that this alternate classloading mechanism kicked in just for the OSGi environment, or whether you are proposing that this new mechanism replaces our current classloading mechanism. Just makes me nervous when I see changes to classloading...
o None of the new files have the Apache license at the top of the file.
o I saw this author tag in one of the files that makes me very nervous...
+ * @author shsmith from EclipseLink
You mentioned that you based this patch off of how EclipseLink did something. We can't just take code from EclipseLink without providing proper credit. To be honest, I'm not sure how the licensing would work in this case. Was this code covered by the Eclipse Public License? Need more investigation and justification before we could ever accept code like this.
o Using splat (*) in the import statements is not sufficient. With the tools available today, you can easily clean this up with the explicit imports.
o Test cases? Not only for the OSGi environment, but if you are affecting current classloading, then what assurances do we have that nothing was affected?
Trying hard not to rain on your parade. Allowing OpenJPA to play in the OSGi environment is important to the project and we appreciate the input. We just have a large set of customers that we need to continue to support.
Thanks,
Kevin
> OSGi Aware Persistence Provider Implementation
> ----------------------------------------------
>
> Key: OPENJPA-1403
> URL: https://issues.apache.org/jira/browse/OPENJPA-1403
> Project: OpenJPA
> Issue Type: New Feature
> Reporter: Milinda Lakmal Pathirage
> Attachments: osgi.patch
>
>
> Current OpenJPA trunk implementation doesn't have full support for using OpenJPA in OSGi containers. For example when OSGi bundle which use OpenJPA deployed, OpenJPA persistence provider cannot locate the persistence.xml in that bundle due to class loading differences in OSGi environment. EclipseLink has resolved this by using bundle listeners and JPA specific OSGi bundle header. Patch provided in this JIRA solve issues in OpenJPA in OSGi environment by following method used in EclipseLink. But there is a problem with current OpenJPA implementation which caused me to add Dynamic-Imports header to OpenJPA OSGi bundle to allow loading classes from bundles that use OpenJPA. I think current OpenJPA implementation doesn't provide support to replace central class load to support loading classes from bundles which use OpenJPA. If we have that support we'll be able to remove Dynamic-Imports and make OpenJPA OSGi bundle follow OSGi best practices .
> Please review the path and provide your ideas about this patch.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (OPENJPA-1403) OSGi Aware Persistence Provider
Implementation
Posted by Milinda Pathirage <mi...@gmail.com>.
Hi Kevin,
Thank you for your valuable feedback about this. I'll fix these and try to
find out solution for eclipse licensing issue.
Thanks,
Milinda
On Wed, Dec 2, 2009 at 5:46 AM, Kevin Sutter (JIRA) <ji...@apache.org> wrote:
>
> [
> https://issues.apache.org/jira/browse/OPENJPA-1403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12784556#action_12784556]
>
> Kevin Sutter commented on OPENJPA-1403:
> ---------------------------------------
>
> Hi Milinda,
> I haven't fully absorbed your patch and its implications yet, but I do have
> some initial comments on the patch content and format...
>
> o Classloading is a touchy area. OpenJPA needs to work in a standalone
> Java SE environment, an application server Java EE environment, and now the
> OSGi bundled environment. As we integrated with the various application
> servers, we had to ensure that the classloading mechanisms continued to work
> in these other environments. I'm not sure how much verification you have
> done with your proposed patch in these other environments. On first glance,
> I didn't see that this alternate classloading mechanism kicked in just for
> the OSGi environment, or whether you are proposing that this new mechanism
> replaces our current classloading mechanism. Just makes me nervous when I
> see changes to classloading...
>
> o None of the new files have the Apache license at the top of the file.
>
> o I saw this author tag in one of the files that makes me very nervous...
>
> + * @author shsmith from EclipseLink
>
> You mentioned that you based this patch off of how EclipseLink did
> something. We can't just take code from EclipseLink without providing
> proper credit. To be honest, I'm not sure how the licensing would work in
> this case. Was this code covered by the Eclipse Public License? Need more
> investigation and justification before we could ever accept code like this.
>
> o Using splat (*) in the import statements is not sufficient. With the
> tools available today, you can easily clean this up with the explicit
> imports.
>
> o Test cases? Not only for the OSGi environment, but if you are affecting
> current classloading, then what assurances do we have that nothing was
> affected?
>
> Trying hard not to rain on your parade. Allowing OpenJPA to play in the
> OSGi environment is important to the project and we appreciate the input.
> We just have a large set of customers that we need to continue to support.
>
> Thanks,
> Kevin
>
> > OSGi Aware Persistence Provider Implementation
> > ----------------------------------------------
> >
> > Key: OPENJPA-1403
> > URL: https://issues.apache.org/jira/browse/OPENJPA-1403
> > Project: OpenJPA
> > Issue Type: New Feature
> > Reporter: Milinda Lakmal Pathirage
> > Attachments: osgi.patch
> >
> >
> > Current OpenJPA trunk implementation doesn't have full support for using
> OpenJPA in OSGi containers. For example when OSGi bundle which use OpenJPA
> deployed, OpenJPA persistence provider cannot locate the persistence.xml in
> that bundle due to class loading differences in OSGi environment.
> EclipseLink has resolved this by using bundle listeners and JPA specific
> OSGi bundle header. Patch provided in this JIRA solve issues in OpenJPA in
> OSGi environment by following method used in EclipseLink. But there is a
> problem with current OpenJPA implementation which caused me to add
> Dynamic-Imports header to OpenJPA OSGi bundle to allow loading classes from
> bundles that use OpenJPA. I think current OpenJPA implementation doesn't
> provide support to replace central class load to support loading classes
> from bundles which use OpenJPA. If we have that support we'll be able to
> remove Dynamic-Imports and make OpenJPA OSGi bundle follow OSGi best
> practices .
> > Please review the path and provide your ideas about this patch.
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
--
Milinda Pathirage
Senior Software Engineer & Product Manager WSO2 BPS; http://wso2.org/bps
WSO2 Inc.; http://wso2.com
E-mail: milinda@wso2.com, milinda.pathirage@gmail.com
Web: http://mpathirage.com
Blog: http://blog.mpathirage.com