Re: [eclipselink-dev] Adding extension points to EclipseLink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]

Re: [eclipselink-dev] Adding extension points to EclipseLink

From: Lukas Jungmann <lukas.jungmann@xxxxxxxxxx>
Date: Sat, 10 Jan 2015 14:34:18 +0100
Delivered-to: eclipselink-dev@xxxxxxxxxxx
List-archive: <https://dev.eclipse.org/mailman/private/eclipselink-dev>
List-help: <mailto:eclipselink-dev-request@eclipse.org?subject=help>
List-subscribe: <https://dev.eclipse.org/mailman/listinfo/eclipselink-dev>, <mailto:eclipselink-dev-request@eclipse.org?subject=subscribe>
List-unsubscribe: <https://dev.eclipse.org/mailman/options/eclipselink-dev>, <mailto:eclipselink-dev-request@eclipse.org?subject=unsubscribe>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.3.0

On 1/9/15 6:39 PM, Rick Curtis wrote:

Lukas - Thanks for the comments. I know better than putting private+final... but it was me being overly paranoid about wrapping methods with doPriv calls.

I'm not sure about these calls neither yet, therefore I'm following existing pattern - as you did in your patch. From time to time I'm running existing tests with security manager on locally to see if something is broken (.. and I've already fixed some issues in this area in the past, ie https://bugs.eclipse.org/bugs/show_bug.cgi?id=434182)

The code I pushed out had both of your suggestions.

thanks!

Do I need a separate defect to have the documentation updated[1] or can I use the same one?

either move existing issue to docs component or file a new issue under docs linking the original one, so the docs team knows there's something to be done. You may want to try to prepare a patch for http://git.eclipse.org/c/www.eclipse.org/eclipselink.git/tree/documentation/2.6/jpa/extensions/persistenceproperties_ref.htm yourself to try to speed it up a bit.

thanks,
--lukas

[1] https://www.eclipse.org/eclipselink/documentation/2.6/jpa/extensions/persistenceproperties_ref.htm#target-server
On Fri, Jan 9, 2015 at 5:23 AM, Lukas Jungmann <lukas.jungmann@xxxxxxxxxx> wrote:
only nit-picking:
- 'private' should be enough on methods, no need to use 'private final' there in WebSpherePlatformDetector
- I'd perhaps rename 'discoverServerPlatform' method to 'detectServerPlatform'

... but that's just my taste (I can live with current version as-is too) ...

The rest looks fine to me too.

Thanks,
--lukas

On 1/8/15 10:40 PM, Rick Curtis wrote:
One last patch.

* Modified the WebSpherePlatformDetector and NoServerPlatformDetector to properly(?) use java2security doPriv blocks. Should I do the same for the WLSDetector?

* Updated the message to be INFO.
On Thu, Jan 8, 2015 at 8:14 AM, andrei ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
I agree, probably INFO.

On 1/7/2015 6:12 PM, Lukas Jungmann wrote:
Hi Rick,

On 1/7/15 11:59 PM, Rick Curtis wrote:

> - Logging
The reason that I used the AbstractSession.logMessage(String) method is that I didn't want to have my message translated... looks like I needed to stumble across TraceLocalizationResource.

I think that platform/server being used should be printed out at INFO (or CONFIG?) level, not FINEST, at some point, so user is informed about what is being used, similarly as it is done when DB platform is detected

thanks,
--lukas
> - Now that the target server is detected just one time

I removed caching from NoServerPlatformDetector

Hopefully here is the last patch.

Thanks,

Rick
On Wed, Jan 7, 2015 at 3:01 PM, andrei ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
Hi Rick,

- Logging: consider using the usual Eclipselink pattern specifying log level, category and using resource bundle, like this one in WebLogic_10_Platform:
abstractSession.log(SessionLog.FINEST, SessionLog.SERVER, "sequencing_connected", null);
- Now that the target server is detected just one time, consider following a simple implementation pattern for ServerPlatformDetectors without static caching the result of checkPlatform (which is now the case for NoServerPlatformDetector);
- I haven't tested WebLogicPlatformDetector, but it should work because this code works elsewhere (WebLogicPlatform.initializeServerNameAndVersion).

Thanks,
Andrei
On 1/7/2015 3:01 PM, Rick Curtis wrote:
Andrei -

Thanks for the review. Can I have you take one last look at the attached patch?

Things to note :

* Added a new javase test that ensures that the detector doesn't override a user configured target-server

* Added some debug in the event that a platform is successfully detected.

* Added WebLogicPlatformDetector per your previous email. Obviously I wasn't able to test that, so I'm just assuming it works.

* ServerPlatformUtils now caches the target server on the first call.

Thanks,

Rick
On Tue, Jan 6, 2015 at 2:09 PM, andrei ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
Hi Rick,

The patch looks good, some comments:
- I think NoServerPlatformDetector.checkPlatform should return TargetServer.None rather than TargetServer.DEFAULT;
- updateServerPlatform typically called two times (once in predeploy, once in deploy) - to detect server platform no more than once consider adding a boolean flag to EntityManagerSetupImpl;
-- actually better to define a static variable EntityManagerSetupImpl.detectedServerPlatformClass and set it the first time auto detection is done - that way detection will be performed just once (except for possible initialization concurrency)
- WLSPlatformDector:
public String checkPlatform() {
    String platform = null;
    try {
      if(this.class.getClassLoader().getClass().getName().contains("weblogic") {
          platform = ServerPlatform.WebLogic;
            Class clazz = PrivilegedAccessHelper.getClassForName("weblogic.version");
            Method method = PrivilegedAccessHelper.getMethod(clazz, "getReleaseBuildVersion", null, false);
            String serverNameAndVersion = (String) PrivilegedAccessHelper.invokeMethod(method, null, null);
            if (Helper.compareVersions(serverNameAndVersion, "9") >= 0) {
              if (Helper.compareVersions(serverNameAndVersion, "10") >= 0) {
            platform = ServerPlatform.WebLogic_10;
                  } else {
            platform = ServerPlatform.WebLogic_9;
                  }
            }
        }
    } catch (Throwable ex){}
    return platform;
}

Thanks,
Andrei
On 1/5/2015 6:21 PM, Rick Curtis wrote:
Andrei -

Please take a look at the patch attached below and let me know what you think. If it seems to be in the right direction, could I have you write a WLSPlatformDector that could be included in the initial commit?

Thanks,

Rick
On Wed, Dec 17, 2014 at 9:21 PM, Andrei Ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
----- Original Message -----
From: curtisr7@xxxxxxxxx
To: eclipselink-dev@xxxxxxxxxxx
Sent: Wednesday, December 17, 2014 4:03:05 PM GMT -05:00 US/Canada Eastern
Subject: Re: [eclipselink-dev] Adding extension points to EclipseLink

I like the idea of having a NoPlatformDetector that is first in line, and checks the ClassLoader to see if it is a basic app classloader. This will minimize the cost for anyone that isn't running in an app server env.

> and we don't know how expensive their checkPlatform calls might be

We would need to keep a very close on eye on PlatformDetectors to ensure that they aren't doing any heavy lifting. This detection mechanism must be lightweight.

Looking at the ClassLoader won't help my case where I need to differentiate between two different WebSphere application servers. I would guess that the ClassLoader path might also have problems differentiating between WLS9 and WLS10?

> If classLoader.getClas().getName() has "weblogic" in it then we immediately call WebLogicPlatformDetrector, which will decide whether its wls9 or wls10.

Thanks,

Rick
On Wed, Dec 17, 2014 at 2:44 PM, Andrei Ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
The drawback is a necessity to loop through all SeverPlatformDetectors (and we don't know how expensive their checkPlatform calls might be).

Alternatively "websphere" word in a package name is a good reason to call WebSpherePlatformDetector right away.

If it's a standard JSE classloader -> NoPlatform right away.

Thanks,

Andrei
----- Original Message -----
From: curtisr7@xxxxxxxxx
To: eclipselink-dev@xxxxxxxxxxx

Sent: Wednesday, December 17, 2014 2:54:19 PM GMT -05:00 US/Canada Eastern
Subject: Re: [eclipselink-dev] Adding extension points to EclipseLink
That is the beauty of the last patch I posted. For each app server we cam do what makes the most sense.

From a WebSphere point of view, I wouldn't look at the classloader class name as those classes are internals and are subject to change at any time. Using some sort of a defined API/SPI/something is the ideal solution. The two WebSphere mechanisms (system prop and a static class) I used in the patch are documented and can't go away / change.

Thanks,

Rick
On Wed, Dec 17, 2014 at 1:29 PM, Andrei Ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
Consider checking the classloader class name - just like driver name in db case that should allow to figure out the app. server vendor.
----- Original Message -----
From: curtisr7@xxxxxxxxx
To: eclipselink-dev@xxxxxxxxxxx
Sent: Wednesday, December 17, 2014 1:52:52 PM GMT -05:00 US/Canada Eastern
Subject: Re: [eclipselink-dev] Adding extension points to EclipseLink

Doug -

Thanks for the followup.

> That way it was automatically populated for us when we were bootstrapping a container managed EMF.

In WebSphere we're planning the same thing. As I mentioned earlier in this thread, this detection support targets the case of application managed (Persistence.createEntityManagerFactory) when running in a container.

> Avoid incorrectly assuming you are in a container when you are not.

It will be up to each Platform to figure out what makes the most sense to make this determination. It is likely that the detection won't be perfect in all cases, but it will be better than what is in place today.

> Do not override a user provided server platform at any time.

100%. We only want to do something in the case where a property has not been specified.

> Allow for an EMF to be created within a container where a server platform integration is not wanted - fringe case but does come up occasionally.

Indeed this is an edge case and with detection in place it would require the user to explicitly set the target-server=None

Thanks,

Rick
On Wed, Dec 17, 2014 at 12:07 PM, Doug Clarke <douglas.clarke@xxxxxxxxxx> wrote:
Rick,

Sorry for joining this conversation late. I do like the idea of detecting the server platform similar to how the database platform is detected. In the case of the database platform we have physical connection details from which we can make a determination. In the case of the server it has been difficult to identify that you are in the container versus on the class path of the container. Its the later case that prevented my earlier work in this area.

In the case of WebLogic the server team did some work to include the server platform if it was not already in the PU properties. That way it was automatically populated for us when we were bootstrapping a container managed EMF.

The situations I believe you need to account for are:

Avoid incorrectly assuming you are in a container when you are not. In the end I was looking at the acquired initial context to see where EclipseLink is actually being run.

Do not override a user provided server platform at any time. Some users want to extend a platform or provide their own for specific cases.

Allow for an EMF to be created within a container where a server platform integration is not wanted - fringe case but does come up occasionally.

I hope this helps,

Doug
On Dec 17, 2014, at 11:31 AM, Rick Curtis <curtisr7@xxxxxxxxx> wrote:
Attached below is a prototype patch with a similar(yet lighter weight) approach. With this approach each app server can implement a lightweight detector that makes sense for a given platform. The patch includes a detector that is able to detect both Liberty and Full profile. The code is structured as such to minimize changes to EntityManagerSetupImpl to keep main line code paths the same.

Let me know what you think about this approach.

Thanks,

Rick
On Mon, Dec 15, 2014 at 4:36 PM, Rick Curtis <curtisr7@xxxxxxxxx> wrote:
After thinking about this some more over the weekend, I'm going to back away from the patch I posted last week. Rather using jndi names to try to determine what platform we're running on, I'm going to try another approach. I'll try to post a patch tomorrow.

Thanks,

Rick
On Fri, Dec 12, 2014 at 2:33 PM, Rick Curtis <curtisr7@xxxxxxxxx> wrote:
Thanks for the idea, but that feels like more of a work around than a solution. I'd prefer to go the route where we attempt to figure out the platform.

Thanks,

Rick
On Fri, Dec 12, 2014 at 1:56 PM, andrei ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
Hi Rick,

I just realized there is a simpler solution - specify target server platform as a system property when starting the app. server.

Thanks,
Andrei
On 12/12/2014 1:45 PM, Rick Curtis wrote:
One more thing, thoughts on logging an INFO message in the event that we detected a target server and it is being used?
On Fri, Dec 12, 2014 at 12:41 PM, Rick Curtis <curtisr7@xxxxxxxxx> wrote:
Andrei -

Thanks for the pointers! I understand trying to avoid polluting core classes with externals. That was the largest concern with the patch I posted.

I don't know why I didn't look into changing the JNDIConnector in the ServerPlatform. I will update the WebSpherePlatform constructor to change that value. Simple fix.

Attached below you'll find a patch where I add support for detecting the server platform if the target-server property hasn't been specified by a user. You'll notice that this detection only works for WebSphere_7 and WebSphere_liberty as those were the platforms I've tested. Should I blindly add in some of the other app servers, or wait to do that till after someone else confirms this approach works for the other servers?

While this solution isn't the most performant, but I don't know of a better route. That being said, I could cache the value returned from ServerPlatformDetector as that will not change from one invocation to the next.

Let me know what you think.

Thanks,

Rick
On Fri, Dec 12, 2014 at 8:51 AM, andrei ilitchev <andrei.ilitchev@xxxxxxxxxx> wrote:
I would not alter the original Eclipselink approach (core classes like JNDIConnector have no knowledge of the outside world).
Not that this approach is bad (it is not), but the original approach is not going away - so the fear is confusion arising from mixture of two opposite approaches.

It would be easy to achieve the same with the existing approach.
Currently ServerPlatform defines initializeExternalTransactionController method, which is called before connecting the session (in DatabaseSessionImpl.preConnectdatasource).
It would be easy to define a new method on ServerPlatform (say, ServerPlatform.initialize), which would be called instead and which would do all necessary server-platform specific initialization of the session:
void initialize() {
initializeExternalTransactionController();
...
}

I think that requirement for the users to specify server platform as a pu property could be removed, too:
Eclipselink used to require database platform to be specified, but now allows for db platform auto detection (see DatabaseSessionImpl.loginAndDetectDatasource).
Even without any Eclipselink changes, in case of container managed pu the correct server platform is easy for container (app. server) to set.

Thanks,
Andrei

On 12/11/2014 3:48 PM, Rick Curtis wrote:
All -

Currently I'm working on two problems when running in WebSphere that I need to come up with a solution for.

1 - When an application uses javax.persistence.Persistence.createEntityManagerFactory(...), they need to specify a WebSphere target server to get transactions (among other things) to work. If they don't specify a target server, the EclipseLink runtime defaults to using the None target server and the app won't interact with our transaction manager. Yes I could require applications to configure eclipselink.target-server, but I would like to remove that requirement.

2 - Again when using javax.persistence.Persistence.createEntityManagerFactory(...) and direct JNDI names, WebSphere users will encounter bug 260383. Numerous places it is suggested to create a SessionCustomizer and then change the JNDIConnector lookup type. While that works, I would also like to remove that requirement.

Since the changes that I'm looking to make are specific to a single application server, I'd like to add an extension point to EclipseLink so I don't need to pollute the codebase with lots of ibm specific code.

I'd like like to solve this problem by adding something to EclipseLink that would use the service loader mechanism to load a service implementation that would be able to change some of the defaults of various EclipseLink internals. At this point this only changes the values of target-server and the JNDIConnector, but I could see it be useful in other areas. Attached below is a patch of some rough prototyping that I did a few days back.

Please take a quick browse through it to see if it seems like a reasonable approach.

Thanks,

Rick

--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
--

Rick Curtis
--

Rick Curtis
<platform.detector.v2.patch>_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________ eclipselink-dev mailing list eclipselink-dev@xxxxxxxxxxx To change your delivery options, retrieve your password, or unsubscribe from this list, visit https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev
--

Rick Curtis
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev

References:
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Andrei Ilitchev
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Rick Curtis
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: andrei ilitchev
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Rick Curtis
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: andrei ilitchev
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Rick Curtis
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Lukas Jungmann
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: andrei ilitchev
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Rick Curtis
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Lukas Jungmann
- Re: [eclipselink-dev] Adding extension points to EclipseLink
  - From: Rick Curtis

Prev by Date: Re: [eclipselink-dev] Adding extension points to EclipseLink
Next by Date: [eclipselink-dev] Bug 456067 - javax.persistence.query.timeout
Previous by thread: Re: [eclipselink-dev] Adding extension points to EclipseLink
Next by thread: Re: [eclipselink-dev] Adding extension points to EclipseLink
Index(es):
- Date
- Thread

Breadcrumbs