Add EventListener to HiveConnector #3358

s2lomon · 2020-04-06T14:25:32Z

opens up the possibility to add external event listeners
that live in hive plugin ecosystem

kokosing

Few minor comments. Otherwise looks good.

kokosing · 2020-04-06T18:38:22Z

presto-hive/src/main/java/io/prestosql/plugin/hive/InternalHiveConnectorFactory.java

+            List<EventListener> eventListeners = injector.getInstance(Key.get(new TypeLiteral<Set<EventListener>>() {}))
+                    .stream()
+                    .map(listener -> new ClassLoaderSafeEventListener(listener, classLoader))
+                    .collect(toImmutableList());


Why not to toImmutableSet()?

there is not point to even attempt any deduplication

I had two points, none of them serious:

injector.getInstance(Key.get(new TypeLiteral<Set<EventListener>>() {})) returns a set, so why do we change this?

Using list here could make a false impression that this could contain duplicates (set still can, but it is less obvious). I don't see a reason why one would need even thing about it.

Any way, I will it leave up to author.

Since ClassLoaderSafeEventListener doesn't (and shouldn't) implement equals, using set is pointless.

kokosing · 2020-04-06T18:39:46Z

presto-hive/src/main/java/io/prestosql/plugin/hive/InternalHiveConnectorFactory.java

@@ -104,6 +109,7 @@ public static Connector createConnector(String catalogName, Map<String, String>
                        binder.bind(PageSorter.class).toInstance(context.getPageSorter());
                        binder.bind(CatalogName.class).toInstance(new CatalogName(catalogName));


Just add newSetBinder(binder, EventListener.class); here. I don't see a need for defaultEventListenerModule() method.

better reading experience along with expressing the intention of it, so that the reader doesn't have to guess that it's a necessary default Event Listener.

It is a matter of taste. I would prefer not to over-optimize reading experience, but rather follow the current layout.

Agreed, we should just bind an empty set here.

kokosing · 2020-04-06T18:41:01Z

...toolkit/src/main/java/io/prestosql/plugin/base/classloader/ClassLoaderSafeEventListener.java

+    private final EventListener delegate;
+    private final ClassLoader classLoader;
+
+    public ClassLoaderSafeEventListener(EventListener delegate, ClassLoader classLoader)


Suggested change

public ClassLoaderSafeEventListener(EventListener delegate, ClassLoader classLoader)

public ClassLoaderSafeEventListener(@ForClassLoaderSafe EventListener delegate, ClassLoader classLoader)

What would that annotation do? This class is used as a decorator inside a stream/for-loop operation. Nothing will be injected into it through guice, as there are potentially many EventListeners etc. For me adding it there would be misleading, but maybe I don't get the full purpose.

ClassLoaderSafe* classes are also designed to be used with guice. I wanted to add this annotation to make sure it follows the convention and behaves like any other ClassLoaderSafe* class.

Please don't get me wrong. I don't want you to use it ;)

ok I get it :)

...toolkit/src/main/java/io/prestosql/plugin/base/classloader/ClassLoaderSafeEventListener.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConnector.java

kokosing · 2020-04-07T11:44:05Z

presto-hive/src/main/java/io/prestosql/plugin/hive/InternalHiveConnectorFactory.java

+    private static Module defaultEventListenerModule()
+    {
+        return binder -> {
+            Multibinder.newSetBinder(binder, EventListener.class)


static import

electrum · 2020-04-07T17:23:40Z

presto-hive/src/main/java/io/prestosql/plugin/hive/InternalHiveConnectorFactory.java

@@ -104,6 +109,7 @@ public static Connector createConnector(String catalogName, Map<String, String>
                        binder.bind(PageSorter.class).toInstance(context.getPageSorter());
                        binder.bind(CatalogName.class).toInstance(new CatalogName(catalogName));


Agreed, we should just bind an empty set here.

electrum · 2020-04-07T17:26:05Z

presto-hive/src/main/java/io/prestosql/plugin/hive/InternalHiveConnectorFactory.java

+    {
+        return binder -> {
+            newSetBinder(binder, EventListener.class)
+                    .addBinding().toInstance(new EventListener() {});


There's no need to create a no-op listener. The default implementation of Connector.getEventListeners() returns an empty set, so empty iterable is legal. We just need to create the set binder.

kokosing · 2020-04-09T08:29:46Z

Merged, thanks!

cla-bot bot added the cla-signed label Apr 6, 2020

kokosing reviewed Apr 6, 2020

View reviewed changes

findepi changed the title ~~Add EventListenes to HiveConnector~~ Add EventListeners to HiveConnector Apr 7, 2020

findepi changed the title ~~Add EventListeners to HiveConnector~~ Add EventListener to HiveConnector Apr 7, 2020

s2lomon force-pushed the hive-plugin-event-listener-extension branch from 9d1fc6c to c6e4a43 Compare April 7, 2020 11:27

kokosing approved these changes Apr 7, 2020

View reviewed changes

Add ClassLoader safe Event Listener

a8e7518

s2lomon force-pushed the hive-plugin-event-listener-extension branch from c6e4a43 to 4eaf459 Compare April 7, 2020 12:25

kokosing approved these changes Apr 7, 2020

View reviewed changes

electrum reviewed Apr 7, 2020

View reviewed changes

Allow HiveConnector to return EventListener

ef9202d

s2lomon force-pushed the hive-plugin-event-listener-extension branch from 4eaf459 to ef9202d Compare April 8, 2020 13:09

kokosing merged commit b5e3aff into trinodb:master Apr 9, 2020

kokosing added this to the 333 milestone Apr 9, 2020

This was referenced Jun 18, 2024

Release notes for 397 #14154

Closed

JDBC Client: fix a bug where TrinoResultSet cannot be properly closed #13775

Merged

Test no thread leaks in engine and connectors #21913

Draft

Fix threads leak in SqlTaskManager #21466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EventListener to HiveConnector #3358

Add EventListener to HiveConnector #3358

s2lomon commented Apr 6, 2020

kokosing left a comment

kokosing Apr 6, 2020

findepi Apr 6, 2020

kokosing Apr 7, 2020

findepi Apr 7, 2020

kokosing Apr 6, 2020

s2lomon Apr 7, 2020

kokosing Apr 7, 2020

electrum Apr 7, 2020

s2lomon Apr 8, 2020

kokosing Apr 6, 2020

s2lomon Apr 7, 2020

kokosing Apr 7, 2020

kokosing Apr 7, 2020

s2lomon Apr 7, 2020

kokosing Apr 7, 2020

electrum Apr 7, 2020

electrum Apr 7, 2020

s2lomon Apr 8, 2020

kokosing commented Apr 9, 2020

		@@ -104,6 +109,7 @@ public static Connector createConnector(String catalogName, Map<String, String>
		binder.bind(PageSorter.class).toInstance(context.getPageSorter());
		binder.bind(CatalogName.class).toInstance(new CatalogName(catalogName));

	public ClassLoaderSafeEventListener(EventListener delegate, ClassLoader classLoader)
	public ClassLoaderSafeEventListener(@ForClassLoaderSafe EventListener delegate, ClassLoader classLoader)

Add EventListener to HiveConnector #3358

Add EventListener to HiveConnector #3358

Conversation

s2lomon commented Apr 6, 2020

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Apr 9, 2020