Added classloader safe configuration of event listener

[gray-eb]

I had an Event Listener plugin that needed to utilize Jersey REST client libraries, but it constantly clashed with the server's libraries. Even after adding a wrapper within the plugin, the issue persisted. It led me to this solution.

This change is analogous to the current state of ConnectorManager.java

Creating a ClassLoaderSafeEventListener similar to the ClassLoaderSafeConnectorMetadata allows me to use the libraries without any conflicts.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[findepi]

two comments:

1. apparently you can apply an equivalent change directly in your own EventListenerFactory implementation. It should not matter if TCCL is set by EventListenerManager before calling your EventListenerFactory or as the first thing in your EventListenerFactory. Please correct me if I am wrong.

2. i think we generally should isolate plugins by default (unless there is a compelling reason not to do so). If we agree, we should fix all the places where we call plugin code, not just single place that happens to be a problem today. Then, we should probably revisit/remove usages of ThreadContextClassLoader in hive connector and other connectors.

[gray-eb]

Just double checked. I am not able to get the same behavior when setting the TCCL in my own factory's implementation of the create() method. It throws the same errors.

This PR was suggested by Dain in this Presto user group thread:
https://groups.google.com/forum/#!topic/presto-users/HPXr7uLcdlc

I agree that this likely needs to be changed in many places, but I am in no position to make those changes myself. I am much more familiar with this part of the code due to my current project. Let me know your thoughts.

[findepi]

i still don't understand why it works in EventListenerFactory and does not work in EventListenerFactory#create. @dain can you possibly explain this to me?
@gray-eb If i wanted to reproduce the problem, what would be the exact steps?

[dain]

@findepi this is the equivilant of the code we have in ConnectorManager (https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/connector/ConnectorManager.java#L319-L321). This is a change @electrum and I suggested to align this code with the other mangers in Presto.

[findepi]

@dain i agree with the change in general (see #11951 (comment) "i think we generally should isolate plugins by default ...")

i remain curious why this works when TCCL is installed before calling the connector and does not work when TCCL is installed as the first thing in the connector code.

[findepi]

The commit message needs improvement. What about:

Set ClassLoader when calling EventListenerFactory

Before the change, certain libraries cannot be used in an event 
listener implementation.

[electrum]

@findepi I suspect it should work in either case. I'm trying to imagine if there could be static initialization which loads classes and would only hit one case, but that seems doubtful as the event factory instance already exists at this point.

[findepi]

@electrum i thought the same

@gray-eb please update the commit message (#11951 (review)) and ping me back.

[gray-eb]

@findepi I've updated the commit message. If you still need steps to reproduce the issue let me know, but I was not able to reproduce the fix within the plugin's EventListenerFactory code.

As some loose steps to reproduce:

• Presto version 0.205
• Create Presto EventListener
• Make simple connection to Atlas via org.apache.atlas.atlas-client-v2, which itself will require the following packages:
  1. org.glassfish.jersey.core.jersey-server
  2. org.glassfish.jersey.core.jersey-container-servlet-core

You should get a LinkageError related to javax.ws.rs-api-2.x classes until you implement the change in this commit.