ConnectionState understanding #36
Conversation
…Q connection state.
Codecov Report
@@ Coverage Diff @@
## master #36 +/- ##
==========================================
+ Coverage 79.11% 79.88% +0.76%
==========================================
Files 9 9
Lines 340 348 +8
==========================================
+ Hits 269 278 +9
Misses 53 53
+ Partials 18 17 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It's a great idea to add such debugging tool. Connection state management in RMQ is a quite complex task so it would be great to have logical representation of it. 👍
I have few comments and question for you.
Also I kindly ask you to add tests for states to be sure that everything will works after future code changes.
mq.go
Outdated
| @@ -19,6 +19,11 @@ const ( | |||
| // Describes states during reconnect. | |||
| statusReadyForReconnect int32 = 0 | |||
| statusReconnecting int32 = 1 | |||
|
|
|||
| ConnectionStateDisconnected int32 = 1 | |||
There was a problem hiding this comment.
It's make sense to add ConnectionStateUnknown = 0 for the default state value.
There was a problem hiding this comment.
Or use ConnectionStateDisconnected as a default value.
mq.go
Outdated
| @@ -45,6 +50,8 @@ type MQ interface { | |||
| Error() <-chan error | |||
| // Close stop all consumers and producers and close connection to broker. | |||
| Close() | |||
| // Shows connection state | |||
| ConnectionState() int32 | |||
There was a problem hiding this comment.
To make it more readable for the client it makes sense to wrap int32 in custom type like ConnectionState. It allows us to add later sugar or completely change implementation without breaking backward compatibility.
mq.go
Outdated
| } | ||
|
|
||
| atomic.StoreInt32(mq.state, ConnectionStateConnecting) |
There was a problem hiding this comment.
Connection state management should be encapsulated in the connect method.
The default value for state flag should be default value of the respective type.
| func (mq *mq) ConnectionState() int32 { | ||
| return atomic.LoadInt32(mq.state) | ||
| } | ||
|
|
||
| func (mq *mq) connect() error { |
There was a problem hiding this comment.
Here we can change status to ConnectionStateConnecting.
mq.go
Outdated
| @@ -429,6 +447,7 @@ func (mq *mq) reconnect() { | |||
|
|
|||
| mq.stopProducersAndConsumers() | |||
|
|
|||
| atomic.StoreInt32(mq.state, ConnectionStateReconnecting) | |||
There was a problem hiding this comment.
I understand the semantic of this status but I'm not sure it will be useful.
Is the difference between connecting and reconnecting states is important for you?
+ test for MQ.ConnectionState()
|
You mentioned everything i did in the beginning, but than changed for some reason 😅 |
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
This test is not useful it only checks that ConnectionState() method will return the value of mq.state.
I think we will get more benefits from tests if we will check state:
- after mq instance creation;
- after successful connect;
- after failed connect;
- after breaking connection;
- after reconnect.
Then we will have some confidence that state works as we designed.
mq.go
Outdated
| @@ -84,8 +86,8 @@ func New(config Config) (MQ, error) { | |||
| producers: newProducersRegistry(len(config.Producers)), | |||
| state: new(int32), | |||
| } | |||
| atomic.StoreInt32(mq.state, int32(ConnectionStateUndefined)) | |||
There was a problem hiding this comment.
Seems like ConnectionStateDisconnected could play the role of a default state (0).
There's no clean way to test connect() method for connection.Channel() error case without refactoring.
|
Nice job! Thank you for the improvement. |
|
I've been published these changes in v.1.2.0 and 1.2.0 versions. |
There are cases when you need to understand what's going on with your MQ connection.
In my case:
i need to understand, that there's no problem with mq connection from my app side and make signal if there are problems with that.
There's no method in current version, that allows you to monitor connection state. Of course we should balance between opening everything and nothing. That's why i added few constants to show connection state.
You may point me, that there's errors channel opened outside
MQ.Error(). But here's the question: how to understand that disconnect / connection problems are not an issues anymore?