give a long gregor reconnect backoff to devices that don't send chats #9720
Conversation
This file contains 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
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
4 times, most recently
from
d139a3a
to
189afc9
Compare
|
Will replace the "dummy revendor" commit after keybase/go-framed-msgpack-rpc#129 lands. |
|
@mmaxim this is ready for your review. |
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
from
189afc9
to
e10089f
Compare
|
Upstream landed, and I've replaced the dummy revendor. Will add a corresponding KBFS PR now. (I expect Android CI will be broken until both of these land.) |
mmaxim
requested changes
Dec 5, 2017
go/chat/server.go
Outdated
| @@ -928,6 +928,8 @@ func (h *Server) PostLocal(ctx context.Context, arg chat1.PostLocalArg) (res cha | |||
| return chat1.PostLocalRes{}, err | |||
| } | |||
|
|
|||
| RecordChatSend(h.G()) | |||
There was a problem hiding this comment.
Could we put this in BlockingSender.Send?
go/chat/active.go
Outdated
| func RecordChatSend(g *globals.Context) { | ||
| err := g.LocalChatDb.PutObj(lastSendTimeDbKey(), nil, time.Now().Unix()) | ||
| if err != nil { | ||
| g.Log.Warning("Failed to store chat last send time: %s", err) |
There was a problem hiding this comment.
Let's make all these prints debugs so they don't show up on the CLI.
go/service/gregor.go
Outdated
| // given a long backoff. | ||
| lastSendTime := chat.GetLastSendTime(g) | ||
| if now.Sub(lastSendTime) < chat.ActiveIntervalAfterSend { | ||
| return GregorConnectionShortRetryInterval |
There was a problem hiding this comment.
Why do we wait at all for active people?
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
4 times, most recently
from
668c717
to
09a7814
Compare
|
Ok, the current state of this PR:
|
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
from
09a7814
to
f980a9f
Compare
|
General comment to use |
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
from
f980a9f
to
840af7d
Compare
|
Made @mmaxim's suggested logging changes and rebased. |
mmaxim
approved these changes
Dec 8, 2017
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
from
840af7d
to
ca9e382
Compare
|
Added a test and rebased. Landing after this passes CI and some hand testing. |
This should mitigate some of the reconnect flood that the gregor servers have to deal with when they restart. Most idle clients in the wild aren't participating in chat, and don't need to reconnect very aggressively. There were a few different heuristics we could've used here, and others we might want to use in the future. One in particular we almost chose was "has this user ever received a message". However, we sometimes send system-wide messages, like when Linux updates are broken, which could confuse that heuristic. Chat sending is a surer sign of activity than receiving, and it also has the benefit of being individual-device-specific. Two things had to change to make this work. First, we had to configure a chat-activity-based backoff (using a couple new keys in LevelDB). Second, we had to make sure that the backoff was respected on reconnect, which required the new ForceInitialBackoff ConnectionOpts parameter upstream, since we don't keep a persisitent Connection object after disconnects.
oconnor663
force-pushed
the
jack/CORE-6527/gregorbackoff
branch
from
ca9e382
to
069968a
Compare
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.
This should mitigate some of the reconnect flood that the gregor servers
have to deal with when they restart. Most idle clients in the wild
aren't participating in chat, and don't need to reconnect very
aggressively.
There were a few different heuristics we could've used here, and others
we might want to use in the future. One in particular we almost chose
was "has this user ever received a message". However, we sometimes send
system-wide messages, like when Linux updates are broken, which could
confuse that heuristic. Chat sending is a surer sign of activity than
receiving, and it also has the benefit of being
individual-device-specific.
Two things had to change to make this work. First, we had to configure a
chat-activity-based backoff (using a couple new keys in LevelDB).
Second, we had to make sure that the backoff was respected on reconnect,
which required the new ForceInitialBackoff ConnectionOpts parameter
upstream, since we don't keep a persisitent Connection object after
disconnects.