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

satellite/audit: create the audit queue #2888

Merged
merged 66 commits into from
Sep 5, 2019
Merged

satellite/audit: create the audit queue #2888

merged 66 commits into from
Sep 5, 2019

Conversation

navillasa
Copy link
Contributor

@navillasa navillasa commented Aug 27, 2019

What:

  • adds an audit queue containing paths to audit
  • adds worker method to ReservoirService to do audit jobs (that is, auditing paths on the queue)

Why:

  • To create clearer organization of the audit system--
    • reservoirChore.Run adds reservoir paths to queue in pseudorandom order
    • audit workers will take a path from the queue and run an audit on that path
    • no duplicate paths will be added to the queue
    • paths will be deleted from the queue as soon as an audit worker picks up the path
    • the random selection from the reservoirs (in pr satellite/audit: random path selection #2861) was a bit convoluted

Please describe the tests:

  • TestChoreAndWorkerIntegration:
    This test ensures that after uploading 2 files (1 remote segment each), the paths for those files appear appropriately in the queue.

  • TestQueue:
    This tests the expected behavior of the Swap and Next methods of the queue struct.

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?

@navillasa navillasa requested a review from a team August 27, 2019 18:39
@cla-bot cla-bot bot added the cla-signed label Aug 27, 2019
@ghost ghost requested review from bryanchriswhite and VinozzZ and removed request for a team August 27, 2019 18:40
@navillasa navillasa added the Request Code Review Code review requested label Aug 27, 2019
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/service.go Outdated Show resolved Hide resolved
satellite/audit/reservoirservice.go Outdated Show resolved Hide resolved
satellite/audit/queue_test.go Outdated Show resolved Hide resolved
satellite/audit/integration_test.go Outdated Show resolved Hide resolved
satellite/audit/integration_test.go Outdated Show resolved Hide resolved
satellite/audit/integration_test.go Show resolved Hide resolved
satellite/audit/reservoirchore.go Outdated Show resolved Hide resolved
satellite/audit/service.go Outdated Show resolved Hide resolved
egonelbre
egonelbre previously approved these changes Sep 5, 2019
Copy link
Member

@egonelbre egonelbre left a comment

Choose a reason for hiding this comment

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

Few nits, but otherwise looks good.

@@ -162,10 +162,14 @@ func (planet *Planet) newSatellites(count int) ([]*satellite.Peer, error) {
MaxExcessRateOptimalThreshold: 0.05,
},
Audit: audit.Config{
Slots: 3,
ChoreInterval: 30 * time.Second,
QueueInterval: 1 * time.Hour,
Copy link
Member

Choose a reason for hiding this comment

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

nit: Let's keep the config organised the same way as the definition.

testQueue2 := []storj.Path{"0", "1", "2"}
q.Swap(testQueue2)

path, err = q.Next()
Copy link
Member

Choose a reason for hiding this comment

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

This can be written as:

for _, expected := range testQueue2 {
    path, err := q.Next()
    require.NoError(t, err)
    require.EqualValues(t, expected, path)
}

)

// ReservoirChore populates reservoirs and the audit queue.
type ReservoirChore struct {
Copy link
Member

Choose a reason for hiding this comment

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

If the other audit thing is going away, this could be named just Chore, because there wouldn't be a naming conflict.

egonelbre
egonelbre previously approved these changes Sep 5, 2019
Copy link
Member

@ifraixedes ifraixedes left a comment

Choose a reason for hiding this comment

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

I think that's worth to wait for @mobyvb review

@navillasa navillasa merged commit 6d363fb into master Sep 5, 2019
@mobyvb mobyvb deleted the green/audit-queue branch September 12, 2019 16:33
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