Add support for multiple query event listeners #24456

evanvdia · 2025-01-29T16:34:39Z

Description

Add support for multiple query event listeners to allow multiple plugins to handle events independently.
Cherry-pick of trinodb/trino#3128

Motivation and Context

Issue : #24454

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Add support for multiple query event listeners.

steveburnett

Thanks for the doc! Could an example be added?

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

steveburnett

Thanks for the doc!

Pull updated branch, new doc build, looks good. Thanks!

tdcmeehan · 2025-01-29T18:12:53Z

Please attribute the author of the commit as mentioned in our contributing guide (see backport commits).

Please write a test with dummy event listeners. You should be able to create internal dummy listeners by writing properties to temp files, invoke the EventListenerManager with a dummy event and prove that two or more events are triggered.

prestodb-ci · 2025-02-05T03:01:28Z

Saved that user @evanvdia is from IBM

ethanyzhang · 2025-02-11T02:57:17Z

Hi @jaystarshot , would it be possible for you to take a look at this? Thank you!!

ethanyzhang · 2025-02-11T03:25:01Z

Sorry I missed Tim's previous comments, will ping the author to update

evanvdia · 2025-02-11T04:31:03Z

@tdcmeehan Added the author of the commit and working on the test case for multiple event listeners.

jaystarshot · 2025-02-11T16:30:56Z

Cherry-pick of https://github.com/prestodb/presto/commit/30e36dae0cdb9debd991931370b86301cd17e261
This is the commit description but that commit is not found

ethanyzhang · 2025-02-12T05:37:45Z

@evanvdia I think the commit to mention in your cherry-pick message should be trinodb/trino@72a7511

Let's update the commit message to the following:

Support multiple event listener plugins

Cherry-pick of https://github.com/trinodb/trino/commit/72a7511a5353cf995d34d6571862f065260b9dfe

Co-authored-by: Jack Dintruff <[email protected]>

evanvdia · 2025-02-13T04:56:55Z

@ethanyzhang update the commit message.

jaystarshot · 2025-02-13T16:07:48Z

presto-main/src/main/java/com/facebook/presto/eventlistener/EventListenerManager.java

+
+    private void createEventListener(File configFile)
+    {
+        log.info("-- Loading event listener %s --", configFile);


private final AtomicBoolean loading = new AtomicBoolean(false);
Loading seems guaraded by an atomic boolean in the trino change

https://github.com/s2lomon/presto/blob/3c7bd73c7c1971069a63a6881f42dd9796606426/presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java#L70

presto-main/src/test/java/com/facebook/presto/eventlistener/TestEventListenerManager.java

aaneja · 2025-03-05T12:55:01Z

presto-main/src/test/java/com/facebook/presto/eventlistener/TestEventListenerManager.java

+
+    private static class EventsCapture
+    {
+        private ImmutableList.Builder<QueryCreatedEvent> queryCreatedEvents;


Make the fields final, and instantiate inline

Suggested change

private ImmutableList.Builder<QueryCreatedEvent> queryCreatedEvents;

private final ImmutableList.Builder<QueryCreatedEvent> queryCreatedEvents = ImmutableList.builder();

private final ImmutableList.Builder<QueryCompletedEvent> queryCompletedEvents = ImmutableList.builder();

private final ImmutableList.Builder<SplitCompletedEvent> splitCompletedEvents = ImmutableList.builder();

public EventsCapture()

{

}

aaneja

LGTM, just some nits about whitespace

aaneja · 2025-03-06T16:14:34Z

presto-main/src/main/java/com/facebook/presto/eventlistener/EventListenerManager.java


-    @VisibleForTesting
-    protected void setConfiguredEventListener(String name, Map<String, String> properties)
+    private void setConfiguredEventListener(String name, Map<String, String> properties)


nit: This method can be moved up to L105 to keep all private methods together

@aaneja Done

evanvdia · 2025-03-07T06:57:29Z

@jaystarshot @pdabre12 Could you please approve the changes.

Cherry-pick of trinodb/trino@72a7511 Co-authored-by: Jack Dintruff <[email protected]>

evanvdia · 2025-03-12T09:39:01Z

@aaneja @jaystarshot @pdabre12 Could you please approve the changes.

aditi-pandit

Thanks. This should help Manta integration.

A previous PR prestodb#24456 added the needed changes to support Multiple event listener in tests, query runners and across the codebase.

evanvdia requested review from a team, elharo and steveburnett as code owners January 29, 2025 16:34

evanvdia requested a review from presto-oss January 29, 2025 16:34

steveburnett requested changes Jan 29, 2025

View reviewed changes

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

evanvdia force-pushed the multiple_event_listener_changes branch from 676726f to 9e29e22 Compare January 29, 2025 17:44

steveburnett previously approved these changes Jan 29, 2025

View reviewed changes

elharo previously approved these changes Jan 31, 2025

View reviewed changes

tdcmeehan added the from:IBM PR from IBM label Feb 5, 2025

prestodb-ci requested review from a team, pdabre12 and sh-shamsan and removed request for a team February 5, 2025 03:01

ethanyzhang requested a review from jaystarshot February 11, 2025 02:57

evanvdia force-pushed the multiple_event_listener_changes branch from 9e29e22 to 928e6c5 Compare February 11, 2025 04:28

evanvdia force-pushed the multiple_event_listener_changes branch from 928e6c5 to b2f27c4 Compare February 11, 2025 16:57

evanvdia force-pushed the multiple_event_listener_changes branch from b2f27c4 to a07f2c3 Compare February 12, 2025 05:48

jaystarshot reviewed Feb 13, 2025

View reviewed changes

aaneja self-requested a review February 17, 2025 06:26

evanvdia dismissed stale reviews from steveburnett and elharo via 5b40300 February 18, 2025 14:00

evanvdia force-pushed the multiple_event_listener_changes branch from a07f2c3 to 5b40300 Compare February 18, 2025 14:00

aaneja requested changes Mar 5, 2025

View reviewed changes

evanvdia force-pushed the multiple_event_listener_changes branch 3 times, most recently from 85bfad5 to 871ff5e Compare March 5, 2025 19:29

evanvdia requested a review from aaneja March 5, 2025 19:30

aaneja previously approved these changes Mar 6, 2025

View reviewed changes

evanvdia dismissed aaneja’s stale review via da6ee5c March 6, 2025 16:46

evanvdia force-pushed the multiple_event_listener_changes branch from 871ff5e to da6ee5c Compare March 6, 2025 16:46

aaneja previously approved these changes Mar 7, 2025

View reviewed changes

evanvdia force-pushed the multiple_event_listener_changes branch from da6ee5c to d5a55f8 Compare March 11, 2025 06:58

evanvdia dismissed aaneja’s stale review via b8c9446 March 11, 2025 09:31

evanvdia force-pushed the multiple_event_listener_changes branch 4 times, most recently from a1b87da to 30f6ae9 Compare March 12, 2025 05:01

Support multiple event listener plugins

a4e9bc4

Cherry-pick of trinodb/trino@72a7511 Co-authored-by: Jack Dintruff <[email protected]>

evanvdia force-pushed the multiple_event_listener_changes branch from 30f6ae9 to a4e9bc4 Compare March 12, 2025 06:57

pdabre12 approved these changes Mar 12, 2025

View reviewed changes

aaneja approved these changes Mar 13, 2025

View reviewed changes

jaystarshot approved these changes Mar 13, 2025

View reviewed changes

aditi-pandit approved these changes Mar 13, 2025

View reviewed changes

aditi-pandit merged commit 7f0f4e1 into prestodb:master Mar 13, 2025
93 checks passed

ScrapCodes mentioned this pull request Mar 19, 2025

Update tests for multiple eventlistener plugin changes #24755

Merged

6 tasks

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

-        private ImmutableList.Builder<QueryCreatedEvent> queryCreatedEvents;
+private final ImmutableList.Builder<QueryCreatedEvent> queryCreatedEvents = ImmutableList.builder();
+        private final ImmutableList.Builder<QueryCompletedEvent> queryCompletedEvents = ImmutableList.builder();
+        private final ImmutableList.Builder<SplitCompletedEvent> splitCompletedEvents = ImmutableList.builder();
+        public EventsCapture()
+        {
+        }

Add support for multiple query event listeners #24456

Add support for multiple query event listeners #24456

Uh oh!

Conversation

evanvdia commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Jan 29, 2025

Uh oh!

prestodb-ci commented Feb 5, 2025

Uh oh!

ethanyzhang commented Feb 11, 2025

Uh oh!

ethanyzhang commented Feb 11, 2025

Uh oh!

evanvdia commented Feb 11, 2025

Uh oh!

jaystarshot commented Feb 11, 2025

Uh oh!

ethanyzhang commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanvdia commented Feb 13, 2025

Uh oh!

jaystarshot Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

evanvdia Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaneja Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

evanvdia Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

aaneja left a comment

Choose a reason for hiding this comment

Uh oh!

aaneja Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

evanvdia Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

evanvdia commented Mar 7, 2025

Uh oh!

evanvdia commented Mar 12, 2025

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

evanvdia commented Jan 29, 2025 •

edited

Loading

ethanyzhang commented Feb 12, 2025 •

edited

Loading