multi: server connection status #53
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guggero
reviewed
Aug 22, 2022
mailbox/client_conn.go
Outdated
| ConnStatusNotConnected ConnStatus = "Not Connected" | ||
| // ClientConnStatusNotConnected means that the client is not connected | ||
| // at all. This is likely due to the mailbox server being down. | ||
| ClientConnStatusNotConnected ClientConnStatus = "Not Connected" |
There was a problem hiding this comment.
To make these constants a bit less clunky, should we just use ClientNotConnected, ClientSessionNotFound? Or ClientStatusNotConnected, ClientStatusSessionNotFound?
mailbox/server_conn.go
Outdated
| // ServerConnStatusNotConnected means that the server is currently not | ||
| // connected to a mailbox. This is either because the session is | ||
| // restarting or because the mailbox server is down. | ||
| ServerConnStatusNotConnected ServerConnStatus = 0 |
There was a problem hiding this comment.
Same here, drop both ConnStatus or just Conn in the constant names since it's probably quite obviously a connection?
itest/server_harness.go
Outdated
| @@ -74,7 +74,7 @@ func (s *serverHarness) start() error { | |||
| ) | |||
|
|
|||
| mailboxServer, err := mailbox.NewServer( | |||
| s.serverHost, connData, grpc.WithTransportCredentials( | |||
| s.serverHost, connData, nil, grpc.WithTransportCredentials( | |||
There was a problem hiding this comment.
Can we add a check to some of our LNC itests that cover these new status callbacks?
Rename the ConnStatus type to ClientConnStatus so that we can add a Server specific Connection status type in a future commit and not confuse the two.
In this commit, the ServerConnStatus type is added. It allows a caller to extract the connection status of the server.
ellemouton
force-pushed
the
serverConnStatus
branch
from
c0c62ef to
1c03f18
Compare
|
thanks @guggero :) updated the names and added a new commit testing both the server and client status in the itests :) |
guggero
approved these changes
Aug 22, 2022
| // ConnStatus is a description of the connection status of the client. | ||
| type ConnStatus string | ||
| // ClientStatus is a description of the connection status of the client. | ||
| type ClientStatus string |
| @@ -119,3 +161,20 @@ func testLargeResponse(t *harnessTest) { | |||
| require.NoError(t.t, err) | |||
| require.Equal(t.t, len(req)*10, len(resp.Resp)) | |||
| } | |||
|
|
|||
| func assertServerStatus(t *harnessTest, status mailbox.ServerStatus) { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add server connection status info so that a caller of the Server can get informed about changes in the Server's connection. The 3 statuses differ from the Client connection statuses. They are as follows:
ServerConnStatusNotConnected: the server is not connected to the mailbox. This means it is either the mailbox is down or that the server is busy re-connecting to it.ServerConnStatusInUse: A client is currently connected to the serverServerConnStatusIdle: the server is connected to the mailbox and ready to accept a connection but there is no client currently connected.