-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP Exchange Architecture #61
Conversation
Created packages for each component and tied them together in the main class
**does not compile**
…ct with. This allows the code to be more testable and lets us plug and play other implementations as needed
func (ex *Exchange) Send(event string, data interface{}) { | ||
ex.mtx.RLock() | ||
for _, fn := range ex.queues[event] { | ||
fn(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be blocking on each event? we shouldn't make assumptions on how long each operation will take. What do you think about executing these in goroutines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. The sender shouldn't be blocked and doesn't really care if the listener received it or not. We will just have to make sure to make all the receivers thread safe so that they don't step on themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…t block the sender
ignoredItems []string | ||
} | ||
|
||
func New(ex exchange.SendListener, ignoredItems []string) (Watcher, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of depending on exchange
in the watcher, what do you think about having a channel in the watcher that is written to whenever there's a change. That keeps things separate and allows callers to decide on behavior rather than this relying on the listener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a side note, this is extended by the fact that we're accepting this SendListener
interface and we're only using the Send
part of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't passing a channel replace the exchange concept? I'm not opposed to the creator managing the delivery of message but my fear is that it will get convoluted when we add in the UI component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the side note we could break up the interface up a little bit:
type Sender interface {
Send(string, interface{})
}
type Listener interface {
Listen(string, func(interface{})
}
type SendListener interface {
Sender
Listener
}
and then pass a Sender
to the watcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it actually would remove the dependency has on the exchange
. It would place the logic of sending events to the exchange in the creator of it (main
in this case). The exchange would still need to be notified just not implicitly by the watcher. Remember, the UI would only simply listen on events from the exchange and the builder would send the status of the build to the exchange. In this case for example, the watcher needs to know the event it's sending to even though the the caller defined it. If we changed the event we are subscribing to on the caller, that would mean a change in the watcher which means there's something up. Think if the watcher wasn't our package but 3rd party.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I understand better now where you're coming form. I can see how that would work for the watcher since it only ever sends one event with no data. However, how would that work for the builder?
The builder sends 3 (potentially a 4th even for logs) events with data (in this case the name). Would it get a chan myStruct
and then parse it accordingly? What you mind making a commit to show how this would look?
What I like about the components using the exchange is the consistency in the type. We could also move the registering of events into the components themselves. For instance the builder could do something like
ex.Listen("rebuild", b.Build)
inside it's constructor which would alleviate the main class from any sort of orchestration (although i'm not sure if this would make it cleaner).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A concern is each component knowing what events they will be triggering. Again thinking of separating these things into packages why should the watcher know that there's an event called rebuild
being listened to on the builder. In any case, for the sake of keeping PRs short and sweet, I think this is a good enough restructure and we'll iterate over it. I'll code up what I mean on my arguments and if we like it we can always merge it or close it if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'm definitely interested in seeing what you said in the code. I agree that the separation of concern is important, I'm just having s little trouble visualizing.
Relates to #21
So far, I've just moved code around. There are still more tests to be written and more cleaning up to be done but I didn't want to do that all at once since this is already a lot of changes.