[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
GitHub user exabrial opened a pull request:

    https://github.com/apache/tomee/pull/175

    [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/exabrial/tomee issues/TOMEE-2249_eclipselink-npe

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/tomee/pull/175.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #175
   
----
commit e5c002ed96911f9c4e2ebf17037eaf7d286ede53
Author: Jonathan S. Fisher <exabrial@...>
Date:   2018-09-29T18:10:49Z

    Add required interface implementations etc

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init

deki
Github user exabrial commented on the issue:

    https://github.com/apache/tomee/pull/175
 
    Fix TOMEE-2249 https://issues.apache.org/jira/projects/TOMEE/issues/TOMEE-2249?filter=reportedbyme by implementing required interfaces and executing method


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user rmannibucau commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r221449894
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/OpenEJBServerPlatform.java ---
    @@ -63,4 +65,11 @@ protected void registerSynchronization_impl(AbstractSynchronizationListener list
                 transaction.registerInterposedSynchronization(synchronization);
             }
         }
    +
    +        @Override
    +        public void prepareServerSpecificServicesMBean() {
    +            if (getDatabaseSession() != null && shouldRegisterRuntimeBean) {
    +                 this.setRuntimeServicesMBean(new MBeanOpenEJBRuntimeServices(getDatabaseSession()));
    --- End diff --
   
    ensure it uses mBeanServer which has a toggle "skip jmx" on the instance


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user rmannibucau commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r221449834
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/MBeanOpenEJBRuntimeServices.java ---
    @@ -0,0 +1,26 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.openejb.jpa.integration.eclipselink;
    +
    +import org.eclipse.persistence.internal.sessions.AbstractSession;
    +import org.eclipse.persistence.sessions.Session;
    +
    +public class MBeanOpenEJBRuntimeServices extends OpenEJBRuntimeServices implements MBeanOpenEJBRuntimeServicesMBean {
    --- End diff --
   
    ideally we would have a single impl, cant we reuse one existing impl since they do nothing here?
   
    only PLATFORM_NAME is set and it is not really needed since only an error thing


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user exabrial commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r223455236
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/MBeanOpenEJBRuntimeServices.java ---
    @@ -0,0 +1,26 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.openejb.jpa.integration.eclipselink;
    +
    +import org.eclipse.persistence.internal.sessions.AbstractSession;
    +import org.eclipse.persistence.sessions.Session;
    +
    +public class MBeanOpenEJBRuntimeServices extends OpenEJBRuntimeServices implements MBeanOpenEJBRuntimeServicesMBean {
    --- End diff --
   
    I certainly can use the Glassfish one here if you would like. But, to remain consistent with how all the other ones are implemented, I would suggest we don't.
   
    ![screen shot 2018-10-08 at 1 18 15 pm](https://user-images.githubusercontent.com/1392297/46626331-a5618080-cafc-11e8-8f13-72905a7debe2.png)



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user exabrial commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r223456172
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/OpenEJBServerPlatform.java ---
    @@ -63,4 +65,11 @@ protected void registerSynchronization_impl(AbstractSynchronizationListener list
                 transaction.registerInterposedSynchronization(synchronization);
             }
         }
    +
    +        @Override
    +        public void prepareServerSpecificServicesMBean() {
    +            if (getDatabaseSession() != null && shouldRegisterRuntimeBean) {
    +                 this.setRuntimeServicesMBean(new MBeanOpenEJBRuntimeServices(getDatabaseSession()));
    --- End diff --
   
    Good idea, I added a check there


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init

deki
In reply to this post by deki
Github user exabrial commented on the issue:

    https://github.com/apache/tomee/pull/175
 
    @rmannibucau If this looks good, could you merge?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user rmannibucau commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r223478337
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/MBeanOpenEJBRuntimeServices.java ---
    @@ -0,0 +1,26 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.openejb.jpa.integration.eclipselink;
    +
    +import org.eclipse.persistence.internal.sessions.AbstractSession;
    +import org.eclipse.persistence.sessions.Session;
    +
    +public class MBeanOpenEJBRuntimeServices extends OpenEJBRuntimeServices implements MBeanOpenEJBRuntimeServicesMBean {
    --- End diff --
   
    Was more thinking about dropping this and keeping only OpenEJBRuntimeServices


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user rmannibucau commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r223478552
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/OpenEJBServerPlatform.java ---
    @@ -26,13 +27,14 @@
     import javax.transaction.Synchronization;
     import javax.transaction.TransactionManager;
     
    -public class OpenEJBServerPlatform extends JMXServerPlatformBase {
    +public class OpenEJBServerPlatform extends JMXServerPlatformBase implements JMXEnabledPlatform {
         public OpenEJBServerPlatform(final DatabaseSession newDatabaseSession) {
             super(newDatabaseSession);
             try {
                 mBeanServer = MBeanServer.class.cast(
                     OpenEJBServerPlatform.class.getClassLoader().loadClass("org.apache.openejb.monitoring.LocalMBeanServer")
                         .getMethod("get").invoke(null));
    +            this.prepareServerSpecificServicesMBean();
             } catch (final Exception e) {
                 // no-op
    --- End diff --
   
    Fail instead of noop?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user exabrial commented on a diff in the pull request:

    https://github.com/apache/tomee/pull/175#discussion_r223483193
 
    --- Diff: container/openejb-jpa-integration/src/main/java/org/apache/openejb/jpa/integration/eclipselink/OpenEJBServerPlatform.java ---
    @@ -26,13 +27,14 @@
     import javax.transaction.Synchronization;
     import javax.transaction.TransactionManager;
     
    -public class OpenEJBServerPlatform extends JMXServerPlatformBase {
    +public class OpenEJBServerPlatform extends JMXServerPlatformBase implements JMXEnabledPlatform {
         public OpenEJBServerPlatform(final DatabaseSession newDatabaseSession) {
             super(newDatabaseSession);
             try {
                 mBeanServer = MBeanServer.class.cast(
                     OpenEJBServerPlatform.class.getClassLoader().loadClass("org.apache.openejb.monitoring.LocalMBeanServer")
                         .getMethod("get").invoke(null));
    +            this.prepareServerSpecificServicesMBean();
             } catch (final Exception e) {
                 // no-op
    --- End diff --
   
    I agree with you; I don't understand why they split it out either, but they literally did it for _every other server_ so there's probably a good reason.
   
    On the fail vs noop, that was there before me. I suggest leaving it so we don't change current behavior (if JMX fails to initialize, we still alow the application to run. Right now JMX is failing.)
   
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 36)                     .getMethod("get").invoke(null));
    > 515306f7ba0 (Jonathan S. Fisher 2018-09-29 13:10:49 -0500 37)             this.prepareServerSpecificServicesMBean();
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 38)         } catch (final Exception e) {
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 39)             // no-op
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 40)         }
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 41)     }
    > 6e2a4f7c419 (Thiago Veronezi    2015-11-23 14:38:43 -0500 42)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee issue #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init

deki
In reply to this post by deki
Github user rmannibucau commented on the issue:

    https://github.com/apache/tomee/pull/175
 
    Ok so let's merge it.
   
    On the fail point we must not start if we are broken so let's drop it also ;)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...

deki
In reply to this post by deki
Github user asfgit closed the pull request at:

    https://github.com/apache/tomee/pull/175


---