Followup multiple eventlistener plugin changes (Fix tests). #24755
Conversation
ScrapCodes
requested review from
shrinidhijoshi,
a team,
jaystarshot,
feilong-liu,
elharo and
ClarenceThreepwood
as code owners
prestodb-ci
requested review from
a team,
auden-woolfson and
imsayari404
and removed request for
a team
|
cc @zac, @aditi-pandit and @evanvdia |
ScrapCodes
force-pushed
the
followup_multiple_eventlistener
branch
from
d846344 to
d3c9ef0
Compare
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedQueries.java
Show resolved
Hide resolved
| List<EventListener> eventListener = getQueryRunner().getEventListeners(); | ||
| assertFalse(eventListener.isEmpty()); | ||
| assertTrue(eventListener.get(0) instanceof TestHiveEventListenerPlugin.TestingHiveEventListener, eventListener.get(0).getClass().getName()); | ||
| return (TestHiveEventListenerPlugin.TestingHiveEventListener) eventListener.get(0); |
| return Optional.of(getEventListeners().get(0)); | ||
| } | ||
|
|
||
| List<EventListener> getEventListeners(); |
There was a problem hiding this comment.
Should we add a log here if there are multiple event listeners?
There was a problem hiding this comment.
Adding a log message here, is not a good convention. What I could do is add an assert, that invoker of this method expects no more than one eventlistener. But that has consequences, if people are upgrading.
There was a problem hiding this comment.
Thanks @ScrapCodes. Have a small nit, and also please address Auden's comments. Otherwise it looks good.
| assertTrue(eventListener.isPresent()); | ||
| assertTrue(eventListener.get() instanceof TestHiveEventListenerPlugin.TestingHiveEventListener, eventListener.get().getClass().getName()); | ||
| return (TestHiveEventListenerPlugin.TestingHiveEventListener) eventListener.get(); | ||
| List<EventListener> eventListener = getQueryRunner().getEventListeners(); |
There was a problem hiding this comment.
Nit : Change variable name to eventListeners
A previous PR added the needed changes to support Multiple event listener. However it missed some changes in tests, query runners etc. So here we fix the follow up, i.e. fix tests etc..
ScrapCodes
force-pushed
the
followup_multiple_eventlistener
branch
from
d3c9ef0 to
00ac731
Compare
|
Hi @aditi-pandit and @auden-woolfson , updated the PR. Please take a look again! |
| @@ -607,10 +607,10 @@ public StatsCalculator getStatsCalculator() | |||
| } | |||
|
|
|||
| @Override | |||
| public Optional<EventListener> getEventListener() | |||
| public List<EventListener> getEventListeners() | |||
There was a problem hiding this comment.
I think this should be Optional<List
There was a problem hiding this comment.
Thanks @jaystarshot for taking a look. I am not fully sure, I understand that. List can have zero elements (an List#isEmpty check similar to Optional#isPresent check), and if we are concerned about not breaking existing users, we added a default Optional<EventListener> in QueryRunner interface Link.
Description
A previous PR
#24456
added the needed changes to support Multiple event listener.
However it missed some changes in tests, query runners etc.
So here we fix the follow up, i.e. fix tests etc..
Motivation and Context
Impact
An API in QueryRunner to get EventListener is marked as Deprecated.
Test Plan
Existing tests.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.