Event driven connection manager - #567
Conversation
56644f1 to
8ea2410
Compare
| */ | ||
|
|
||
| package stats | ||
| package dto |
There was a problem hiding this comment.
Why do we need to move it into a separate dto package it contains the only struct?
There was a problem hiding this comment.
the stats structure is widely used in the project, we end up in circular references. Since it's client(consumer) related it makes sense to keep it under the client package. So I moved it into its own package :)
There was a problem hiding this comment.
dto name is really bad here as its too generic and technical also it makes you always alias imports because of its name. Consider moving this stats elsewhere. For example /core/consumer/statistics.go
There was a problem hiding this comment.
moved to core/consumer/statistics.go
8ea2410 to
1e64372
Compare
1e64372 to
dfde202
Compare
tadovas
left a comment
tadovas
left a comment
There was a problem hiding this comment.
Main concern - code organisation.
💯 points for finally cleaning up openvpn integration (possibly a chance to improve go-openvpn)
| */ | ||
|
|
||
| package connection | ||
| package session |
There was a problem hiding this comment.
Strange package name. Why "connection" was bad? Also core -> client rename. As I mentioned earlier, there is no client/server. Consumer/provider
There was a problem hiding this comment.
We should use appropriate terms defined by functionality. Connection usually means lower level transmission facility whereas session would imply user (session id) and / or authentication somewhere.
There was a problem hiding this comment.
Let's not forget that term usage heavily depends on context in which that term is used (bounded context). In our case connection means established connection between consumer and provider with all ongoing message channels, underlying transport and accounting
There was a problem hiding this comment.
In our case connection (and manager) is high level definition directly controlled by consumer ( through api).
There was a problem hiding this comment.
It wasn't bad - it just seemed appropriate to move all consumer related event subscribers to a package, in this case - consumer(former client package).
There was a problem hiding this comment.
I've left it under consumer/session for now - if we need a better place for it, #572 is the place to do it.
| */ | ||
|
|
||
| package stats | ||
| package dto |
There was a problem hiding this comment.
dto name is really bad here as its too generic and technical also it makes you always alias imports because of its name. Consider moving this stats elsewhere. For example /core/consumer/statistics.go
| } | ||
|
|
||
| func (cff *connectionFactoryFake) CreateConnection(connectionParams ConnectOptions, stateChannel StateChannel) (Connection, error) { | ||
| func (cff *connectionFactoryFake) CreateConnection(connectionParams ConnectOptions, stateChannel StateChannel, statisticsChannel StatisticsChannel) (Connection, error) { |
There was a problem hiding this comment.
Is this part of tests? It should not be separate file, also as for stubing it looks too complex?
There was a problem hiding this comment.
I'll move this to the stubs file (each and every single one of those had a separate file in the core/connection), but I'd rather not rewrite tests along the pull request that changes a whole lot in the package it's testing. Since the tests were stable for a long time, minimal changes to them assures that one does not break anything with the PR. It might be a candidate for a future refactor though.
b9e94a7 to
c6f7392
Compare
c6f7392 to
529858b
Compare
529858b to
ca59f8d
Compare
tadovas
left a comment
tadovas
left a comment
There was a problem hiding this comment.
So far so good. Just make sure TODOs are registered and we can move on
4 participants
Connection manager now emits events and stats keeper/sender/session storage subscribe to those.
This means that the connection manager gets rid of 3 dependencies, many of the interfaces get thinner.
As a side note - when moving the stats keeper away from openvpn I had to come up with a way for getting stats out from the connection to the manager, therefore a stats channel has been introduced. This ruined the mobile client entrypoint, but I'll fix that separately once this get's rejected -> fixed -> approved -> merged since it might need a change on go-openvpn.
Please do a thorough review on this one. Really appreciate the opinions/ideas/suggestions.
Take note that the "moved the session storage to client package" commit moves the session storage related stuff to the client package. It seemed like the best place to put it, since after the event driven connection manager change it does not seem to fit in the connection package anymore.
This is also based on the decouple-stats-keeper - so keep that in mind. Once this get's merged in, I'll do some work to rebase the decouple-stats-keeper on master, where I'm expecting quite a few conflicts.Bonus: wrote session storage tests.
EDIT: changed base to master, will now update this branch with master.