Unified Edge network queue and Added response event dispatch and handling#173
Unified Edge network queue and Added response event dispatch and handling#173addb merged 14 commits intoadobe:consentfrom
Conversation
…ules. Updated processEvent signature to return responseEvent. ProcessQueuedRequests will be called to process the queue, make network requests and get responses back. Edge responses are now returned as Edge response events and not generic response events.
…way for the other modules who queue the requests with edge to listen for responses. All the modules will get responses for all the edge requests going out and they can choose to handle the request types they wish to. Added an array or registered modules for responses. All the registered modules need to have processResponseEvent method.
…Updated Media module to use the new unified queue architecture and handle the responses from event processor. Updated tests with the changes.
…nifiedNetworkQueue
| processResponseEvent: function(event as object) as void | ||
| _adb_logInfo("MediaModule::processResponseEvent() - Received response event:(" + chr(10) + FormatJson(event) + chr(10) + ")") | ||
|
|
||
| m._sessionManager.handleResponseEvent(event) |
There was a problem hiding this comment.
If possible, can you add a check here so that mediaModule only handles media response?
There was a problem hiding this comment.
In the current state it will be looking at handles or MediaModule being aware of the requestIds. We could add additional property to responses like (parentModule or something like that) which can be used to filer or do some more logic while creating responses like sending each handle as a separate event.
I would like to take this as a separate PR probably during this: #175
There was a problem hiding this comment.
We don’t need to add parentModule. Simply checking the handle will allow the media to exit early.
…i request events are processed.
code/main/media/mediaSession.brs
Outdated
| end if | ||
|
|
||
| ''' only handle the response for sessionStart event | ||
| if m._sessionStartHit.requestId <> requestId |
There was a problem hiding this comment.
Is m._sessionStartHit guaranteed to not be invalid? I prefer adding a check as the responseEvents are processed by all media session objects.
There was a problem hiding this comment.
Mostly yes, since we will reach here only if there is an active session. I will still add null check for this for any corner cases (in case sessionStart hit is not created)
|
|
||
| ''' TODO cleanup (clean up Media public API data and MediaModule) | ||
| ''' Validates the event data for createMediaSession request event | ||
| _isValidEventDataForSessionStartRequest: function(data as object) as boolean |
There was a problem hiding this comment.
I prefer exposing these two helpers from the media module rather than defining them in this class. You can include this change as part of your media cleanup..
There was a problem hiding this comment.
YesI have some cleanups regarding similar things. Added #175
|
|
||
| ' target: _adb_IdentityResponseEvent() | ||
| ' @Test | ||
| sub TC_adb_IdentityResponseEvent() |
There was a problem hiding this comment.
Some of these comparisons will become simpler if we add a test utility method to compare objects/JSON instances. Can you create a GH issue to track this?
Description
Currently Media and Edge modules have a separate queue to handle network requests and responses. Which has problems like event ordering and also makes it difficult to control the events when we introduce consent. And it would also increase repeated logic like extracting Konductor responses like state:store and updating it separately for each queue.
Unified queue gives more control by ensuring order of requests. We could enforce consent more easily and also we could send consent updates in the same queue. Also all the responses from Konductor will be extracted by edge and be appended by edge. All the other modules don't need to worry about general Konductor configuration, state , location hint and even consent. Edge will handle everything.
This would improve the architecture and make it easy to add more modules in future like messaging, personalization etc.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: