pool/store: Fix inverted logic on inactive deadline checks#74
Merged
shazow merged 3 commits intovipnode:masterfrom Sep 25, 2019
Merged
pool/store: Fix inverted logic on inactive deadline checks#74shazow merged 3 commits intovipnode:masterfrom
shazow merged 3 commits intovipnode:masterfrom
Conversation
Collaborator
Author
|
I added another fix in d2f02e2 for the memory pool, as stopping an agent didn't seem to have any effect on trigger inactive peers, because the pool was using the wrong timestamp for when a peer was last seen. I'm not familiar enough w/ Badger to make the same change on the badger store, so figured I'd leave that for you. With this change I now correctly see other agents considering a peer invalid when the peers vipnode-agent has been stopped for a couple minutes, and then everything recovers when the agent is restarted. |
…ally inactive peers.
Collaborator
Author
|
Oops, and 9cbe3ce fixes the unit test failing since .LastSeen was the 0 value. |
shazow
approved these changes
Sep 25, 2019
| inactiveDeadline := now.Add(-store.ExpireInterval) | ||
| for nodeID, timestamp := range nodePeers { | ||
| if timestamp.Before(inactiveDeadline) { | ||
| if timestamp.After(inactiveDeadline) { |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I've tested this on our local cluster and it seems to resolve the surprise
InactivePeers. I think I see now what was happening:numUpdated == len(node.peers)check was short-circuiting the inactive check.I first tested this by leaving it as
.Beforebut removing the short-circuit, and the peering looked as unstable immediately as it did during the peering issues we'd seen:In the above graph the pool w/o the short-circuit was running from ~19:07 - 19:12 UTC, and the code in this PR was running on the pool server at 19:13 onwards, the peering was remaining quite stable since, but has only been running for ~1h now.
I haven't checked the unit tests yet as to why they were passing before, but my guess is they weren't fully exercising the expiration timing.