Fix TestingTrinoServer connector event listener registration#13360
Fix TestingTrinoServer connector event listener registration#13360wendigo wants to merge 2 commits intotrinodb:masterfrom
Conversation
wendigo
force-pushed
the
serafin/register-connector-event-listeners-for-tests
branch
from
a425385 to
f4bcf1b
Compare
This reverts commit 1e7d89c. Longer timeout is not needed.
wendigo
force-pushed
the
serafin/register-connector-event-listeners-for-tests
branch
from
a8b4411 to
ed8c639
Compare
|
|
Created #13365 with the fixup / without the change to the timeout |
| updateConnectorIdAnnouncement(announcer, catalogHandle, nodeManager); | ||
|
|
||
| EventListenerManager eventListenerManager = injector.getInstance(EventListenerManager.class); | ||
| connectorManager.getConnectorServices(catalogHandle).getEventListeners().forEach(eventListenerManager::addEventListener); |
There was a problem hiding this comment.
Do we need product tests to make sure that event listeners are truly registered?
There was a problem hiding this comment.
Server correctly registered EventListeners, TestingTrinoServer did not, so it only affected (some) tests
|
|
||
| QueryEvents queryEvents = eventsCollector.getQueryEvents(queryId); | ||
| queryEvents.waitForQueryCompletion(new Duration(30, SECONDS)); | ||
| queryEvents.waitForQueryCompletion(new Duration(3, SECONDS)); |
There was a problem hiding this comment.
How is that possible that longer timeout fixed the issue? If there was no event listener registered then it should not work at all? Are you referring here to a different problem?
According #13321 it should be 30 seconds.
There was a problem hiding this comment.
#13321 (comment) - no effect but considered an improvement.
FYI - #13370 - This PR is probably superseded @wendigo @kokosing
|
This is fixed in Trino trunk now |
What's the fix? (especially in the context of #3364 (comment) ) |
|
See #13370 the connector event listener was not registered in the engine. Only in tests |
|
I doubt this issue has anything with flakiness. However there were other changes in Trino related to event listener that could be related. |
wendigo
deleted the
serafin/register-connector-event-listeners-for-tests
branch
5 participants
Tests like
TestConnectorEventListener.testConnectorEventHandlerReceivingEventsare broken on master because connector's event listeners are never registered whencreateCatalogis called onTestingTrinoServer(changed since ff323dc)