-
Notifications
You must be signed in to change notification settings - Fork 690
[tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tomee-7.0.x][TOMEE-2249] Fix EclipseLink NPE on init #175
Conversation
|
Fix TOMEE-2249 https://issues.apache.org/jira/projects/TOMEE/issues/TOMEE-2249?filter=reportedbyme by implementing required interfaces and executing method |
rmannibucau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if service code can be dropped, looks like it brings no override, isnt it?
...rc/main/java/org/apache/openejb/jpa/integration/eclipselink/MBeanOpenEJBRuntimeServices.java
Show resolved
Hide resolved
...tion/src/main/java/org/apache/openejb/jpa/integration/eclipselink/OpenEJBServerPlatform.java
Outdated
Show resolved
Hide resolved
|
@rmannibucau If this looks good, could you merge? (also probably want to do this is 7.1.x and 8.0.x) |
rmannibucau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still dont get this jmx service split and the seevice can use the parent class, no need to type it in the current form of this pr
...rc/main/java/org/apache/openejb/jpa/integration/eclipselink/MBeanOpenEJBRuntimeServices.java
Show resolved
Hide resolved
| .getMethod("get").invoke(null)); | ||
| this.prepareServerSpecificServicesMBean(); | ||
| } catch (final Exception e) { | ||
| // no-op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail instead of noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 36) .getMethod("get").invoke(null));
515306f (Jonathan S. Fisher 2018-09-29 13:10:49 -0500 37) this.prepareServerSpecificServicesMBean();
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 38) } catch (final Exception e) {
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 39) // no-op
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 40) }
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 41) }
6e2a4f7 (Thiago Veronezi 2015-11-23 14:38:43 -0500 42)
|
Ok so let's merge it. On the fail point we must not start if we are broken so let's drop it also ;) |
…patch updated release notes for 8.0.9-TT.4 release
No description provided.