Support multiple event listener plugins by jdintruff · Pull Request #2305 · trinodb/trino

jdintruff · 2019-12-18T16:27:31Z

Per #2299 it would be helpful to be able to declare both multiple instances of the same event listener as well as multiple instances of different event listeners. I've tested this locally with two simple logging plugins but I still need to write proper tests.

cla-bot · 2019-12-18T16:27:34Z

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 submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

martint · 2019-12-18T17:07:37Z

@cla-bot check

cla-bot · 2019-12-18T17:07:41Z

The cla-bot has been summoned, and re-checked this pull request!

electrum · 2019-12-18T17:23:40Z

presto-docs/src/main/sphinx/develop/event-listener.rst

We should do this like the recent access control change, where each file name is listed. Unlike catalogs, there’s no concept of “instance name” used by Presto, so we can reference the files directly. See https://github.com/prestosql/presto/blob/master/presto-main/src/main/java/io/prestosql/security/AccessControlConfig.java and https://github.com/prestosql/presto/blob/master/presto-main/src/main/java/io/prestosql/security/AccessControlManager.java

I updated the doc here to reflect the new conf changes

electrum · 2019-12-18T17:24:48Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

Please model the loading code and logging after AccessControlManager

I updated this to be more like the AccessControlManager

electrum · 2019-12-18T17:27:01Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

I’m thinking we should catch exceptions and log a warning, so that one listener doesn’t affect others (especially since there is no defined order they will be invoked)

Added a warning for when exceptions are thrown from the event listeners but I believe this would still allow event listeners to block one another

Yes, being slow or hanging is a problem, but there's not much that can be done. We'd need separate executors for each listener with a fixed queue size that eventually discards events.

electrum

Apologies for the late follow up. A few minor comments, otherwise ready to go. Thanks for implementing this feature.

electrum · 2020-01-27T04:27:03Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

+    private final List<File> configFiles;
    private final Map<String, EventListenerFactory> eventListenerFactories = new ConcurrentHashMap<>();
-    private final AtomicReference<Optional<EventListener>> configuredEventListener = new AtomicReference<>(Optional.empty());
+    private final AtomicReference<List<EventListener>> configuredEventListeners =


This wrapping shouldn't change

electrum · 2020-01-27T04:27:08Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java


        if (eventListenerFactories.putIfAbsent(eventListenerFactory.getName(), eventListenerFactory) != null) {
-            throw new IllegalArgumentException(format("Event listener '%s' is already registered", eventListenerFactory.getName()));
+            throw new IllegalArgumentException(


electrum · 2020-01-27T04:30:42Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

-        checkState(!isNullOrEmpty(name), "Access control configuration %s does not contain '%s'", configFile, NAME_PROPERTY);
-
-        setConfiguredEventListener(name, properties);
+        List<EventListener> eventListeners =


Formatting nit: don't wrap the assignment, but do wrap the stream operations. Also, prefer the immutable collectors.

List<EventListener> eventListeners = configFiles.stream() .map(this::createEventListener) .collect(toImmutableList());

electrum · 2020-01-27T04:35:49Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

+            }
+            configFiles = ImmutableList.of(CONFIG_FILE);
        }
-


Just to be safe, let's add an AtomicBoolean so that we can't accidentally load twice

checkState(loading.compareAndSet(false, true), "Event listeners already loaded");

electrum · 2020-01-27T04:37:04Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

-        log.info("-- Loaded event listener %s --", name);
+        String name = properties.remove(EVENT_LISTENER_NAME_PROPERTY);
+        checkArgument(!isNullOrEmpty(name), "EventListener plugin configuration for %s does not contain %s", configFile,
+                EVENT_LISTENER_NAME_PROPERTY);


Nit: don't wrap here

electrum · 2020-01-27T04:42:24Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java

+            try {
+                listener.queryCompleted(queryCompletedEvent);
+            }
+            catch (Exception e) {


This should be RuntimeException since the event listener doesn't throw any checked exceptions. However, I'm thinking we should make this Throwable. While we hope plugins don't throw Error, if they do, there's nothing we can do, so we might as well log it and continue.

Same for the other events.

electrum · 2020-01-27T04:44:02Z

presto-main/src/main/java/io/prestosql/eventlistener/EventListenerModule.java

    {
+        configBinder(binder).bindConfig(EventListenerConfig.class);
        binder.bind(EventListenerManager.class).in(Scopes.SINGLETON);
+        newExporter(binder).export(EventListenerManager.class).withGeneratedName();


No need to add this since EventListenerManager doesn't have any @Managed annotations.

electrum · 2020-01-27T04:51:45Z

presto-main/src/main/java/io/prestosql/testing/TestingEventListenerManager.java

        extends EventListenerManager
 {
-    private final AtomicReference<Optional<EventListener>> configuredEventListener = new AtomicReference<>(Optional.empty());
+    private final AtomicReference<Set<EventListener>> configuredEventListeners = new AtomicReference(new HashSet());


Use new HashSet<>() so that it's not a raw type.

We don't need to change this class at all, since it still only supports a single event listener. If we want to change it, we should make it add each new listener (instead of overwriting the set). However, I feel changing the test implementation is somewhat unrelated to the rest of the PR (which is allow multiple listeners for the main server).

electrum · 2020-01-27T04:52:04Z

presto-main/src/test/java/io/prestosql/eventlistener/TestEventListenerConfig.java

+    @Test
+    public void testDefaults()
+    {
+        ConfigAssertions.assertRecordedDefaults(ConfigAssertions.recordDefaults(EventListenerConfig.class)


Nit: static import all the ConfigAssertions methods

electrum · 2020-01-27T04:53:31Z

presto-main/src/test/java/io/prestosql/eventlistener/TestEventListenerConfig.java

+                .build();
+
+        EventListenerConfig expected = new EventListenerConfig()
+                .setEventListenerFiles("a,b,c");


Call the List version of the setter in the test, in order to verify the splitting workers correctly. (the value from the map will go through the string version)

tooptoop4 · 2020-03-01T12:46:51Z

some conflicts @jdintruff

ankitdixit · 2020-03-13T11:57:27Z

@jdintruff just wanted to ask if you plan to look at this in near future.

kokosing · 2020-03-13T12:46:50Z

@jdintruff If you don't have time to work on this, I can take over this.

kokosing · 2020-03-17T16:04:57Z

Replaced with #3128

cla-bot bot added the cla-signed label Dec 18, 2019

electrum reviewed Dec 18, 2019

View reviewed changes

jdintruff force-pushed the multiple-event-listeners branch 2 times, most recently from 58876c2 to 63805ff Compare January 3, 2020 08:19

jdintruff marked this pull request as ready for review January 3, 2020 08:19

Support multiple event listener plugins

d9aaf09

jdintruff force-pushed the multiple-event-listeners branch from 63805ff to d9aaf09 Compare January 3, 2020 11:31

electrum reviewed Jan 27, 2020

View reviewed changes

s2lomon mentioned this pull request Mar 17, 2020

Support multiple event listener plugins #3128

Merged

kokosing closed this Mar 17, 2020

Conversation

jdintruff commented Dec 18, 2019

Uh oh!

cla-bot bot commented Dec 18, 2019

Uh oh!

martint commented Dec 18, 2019

Uh oh!

cla-bot bot commented Dec 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tooptoop4 commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitdixit commented Mar 13, 2020

Uh oh!

kokosing commented Mar 13, 2020

Uh oh!

kokosing commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

tooptoop4 commented Mar 1, 2020 •

edited

Loading