[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
Github user asfgit closed the pull request at: https://github.com/apache/tomee/pull/175 ---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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 Veronezi2015-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 Veronezi2015-11-23 14:38:43 -0500 38) }
catch (final Exception e) {
> 6e2a4f7c419 (Thiago Veronezi2015-11-23 14:38:43 -0500 39)
// no-op
> 6e2a4f7c419 (Thiago Veronezi2015-11-23 14:38:43 -0500 40) }
> 6e2a4f7c419 (Thiago Veronezi2015-11-23 14:38:43 -0500 41) }
> 6e2a4f7c419 (Thiago Veronezi2015-11-23 14:38:43 -0500 42)
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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?
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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.
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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
---
[GitHub] tomee pull request #175: [tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on in...
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 Date: 2018-09-29T18:10:49Z Add required interface implementations etc ---
