Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storagenode/contact: create chore for nodes to ping satellites #2877

Merged
merged 35 commits into from
Sep 6, 2019

Conversation

jenlij
Copy link
Contributor

@jenlij jenlij commented Aug 27, 2019

What: Creates a chore for nodes to announce themselves to their trusted satellites. Runs on startup and every hour thereafter

Why: Kademlia Removal

Please describe the tests:

  • Test 1: basic tests of gRPC service
  • Test 2: assert that lastPing time is updated when node contact endpoint is hit

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@cla-bot cla-bot bot added the cla-signed label Aug 27, 2019
@jenlij
Copy link
Contributor Author

jenlij commented Aug 27, 2019

Make sure to update storj-sim and testplanet

@cam-a cam-a added the WIP Work In Progress label Sep 5, 2019
storagenode/contact/chore.go Outdated Show resolved Hide resolved
storagenode/contact/chore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thepaul thepaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

storagenode/contact/endpoint.go Outdated Show resolved Hide resolved
storagenode/contact/chore.go Show resolved Hide resolved
storagenode/contact/chore.go Outdated Show resolved Hide resolved
storagenode/contact/chore.go Outdated Show resolved Hide resolved
storagenode/contact/chore.go Show resolved Hide resolved
@cam-a cam-a marked this pull request as ready for review September 6, 2019 15:51
@cam-a cam-a requested a review from a team September 6, 2019 15:51
@ghost ghost requested review from Barterio and JessicaGreben and removed request for a team September 6, 2019 15:51
@cam-a cam-a added Request Code Review Code review requested and removed WIP Work In Progress labels Sep 6, 2019
@cam-a cam-a requested a review from thepaul September 6, 2019 15:52
@cam-a cam-a changed the title storagenode/outreach: create service for nodes to announce themselves storagenode/contact: create chore for nodes to ping satellites Sep 6, 2019
}

// PingStats contains information regarding who and when the node was last pinged
type PingStats struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to track when/who the node was last pinged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for the node dashboard

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the dashboard indicates the last time you were contacted to let you know you are still externally accessible

func (stats *PingStats) WhenLastPinged() (when time.Time, who storj.NodeID, addr string) {
stats.mu.Lock()
defer stats.mu.Unlock()
return stats.lastPinged, stats.whoPingedNodeID, stats.whoPingedAddress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this method WhenLastPinged isn't currently being used anywhere. Is there a use case for this method to be used in the future? Im just thinking that if the node restarts for any reason, pingStats will get reset and these fields will be empty. Not sure if that will affect anything

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be used by the dashboard to get the last contacted time. I'm also using it in the test

}
}

// Run the contact chore on a regular interval with jitter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we should expand on this comment a little bit and explain what is meant by jitter

@cam-a cam-a merged commit 3387750 into master Sep 6, 2019
@cam-a cam-a deleted the purple/node-outreach-service branch September 6, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants