Implement Event Broker for Async Events and Responses #28
Conversation
Event bus See merge request mind-framework/asterisk/goami!1
|
This should fix #14 too |
| /* | ||
| * El broker de eventos permite leer los eventos de asterisk y distribuirlos entre los subscriptores | ||
| * Esta implementación está basada en https://github.com/kljensen/golang-html5-sse-example/blob/master/server.go | ||
| */ |
There was a problem hiding this comment.
Consider writing all comments in English
| @@ -18,6 +18,7 @@ type Socket struct { | |||
| incoming chan string | |||
| shutdown chan struct{} | |||
| wg sync.WaitGroup | |||
| events *EventsBroker | |||
There was a problem hiding this comment.
I can't see why it's necessary have an event broker in the socket struct.
|
|
||
| import ( | ||
| "context" | ||
| _log "github.com/rs/zerolog/log" |
There was a problem hiding this comment.
Please remove all log messages, before submit the patch. The main idea of goami is working as a library and returns all errors as a normal return value
| return nil, err | ||
| } | ||
| } | ||
| actionID := rsp.Get("ActionID") |
There was a problem hiding this comment.
I agree that ActionID should be the same of included in the request and the verification must be included as fix.
There was a problem hiding this comment.
i will create an new pull request with this fix
| ``` | ||
| ### Advantages of using Event Broker | ||
| * Multiple subscribers (ie: http request por events) | ||
| * The Action ID from response is compared width the action ID from command, so you have accurate response |
There was a problem hiding this comment.
Agree with pros, but the event broker IMHO should be implemented at the top of library, not inside it.
For example:
https://github.com/heltonmarx/goami/blob/master/example/asterisk.go#L90
Consuming the ami.Events(ctx, socket) by a dedicated socket.
And using another socket connection to submit commands only.
Creating a collection of sockets + events dedicated to your application, but not necessarily included to the library.
There was a problem hiding this comment.
the main reason why I put the event broker inside the library was because of the risk of a command "stealing" the response belonging to another command, especially in multi-client environments (rest api).
Let me figure how to solve that problem without including an event broker.
|
ill close this pull request for making some fixes to the code. |
Hi, this pull request implements asynchronous events in AMI.
This should fix issue #22
I have not much experience in contributions, so any recommendation is welcome
BTW, not English speaker here :)