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

New spanner db structure #6061

Merged
merged 41 commits into from Jun 2, 2020
Merged

New spanner db structure #6061

merged 41 commits into from Jun 2, 2020

Conversation

shayzluf
Copy link
Contributor

@shayzluf shayzluf commented May 31, 2020

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
In order to be able to remove the look-back value there is a need to reduce the complexity of the spanner db(less indices)
This pr replaces the index per validator per epoch with one record per epoch

Which issues(s) does this PR fix?

Part Of #5854

BenchmarkStore_SetValidatorSpan-12    	 4998086	       235 ns/op	     208 B/op	       5 allocs/op
BenchmarkStore_GetValidatorSpan-12    	 5699593	       205 ns/op	     192 B/op	       3 allocs/op
BenchmarkStore_EpochSpans-12    	 1169184	      1023 ns/op	     696 B/op	      12 allocs/op
BenchmarkStore_SaveEpochSpans-12    	      57	  27378496 ns/op	10296499 B/op	     101 allocs/op```

@shayzluf shayzluf requested a review from a team as a code owner May 31, 2020 16:02
@shayzluf shayzluf self-assigned this May 31, 2020
@shayzluf shayzluf added the Ready For Review A pull request ready for code review label May 31, 2020
go.mod Outdated
@@ -28,7 +28,6 @@ require (
github.com/go-yaml/yaml v2.1.0+incompatible
github.com/gogo/protobuf v1.3.1
github.com/golang/gddo v0.0.0-20200528160355-8d077c1d8f4c
github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what this is? Maybe @prestonvanloon knows?

slasher/db/iface/interface.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
@shayzluf
Copy link
Contributor Author

shayzluf commented Jun 1, 2020

@0xKiwi added the benchmark results

type EpochSpans interface {
SetValidatorSpan(ctx context.Context, idx uint64, newSpan detectionTypes.Span) error
GetValidatorSpan(ctx context.Context, idx uint64) (detectionTypes.Span, error)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should go in slasher/types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think its good here. maybe another db impl will need to move it to db somehow

slasher/db/kv/kv.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
slasher/db/kv/spanner_new.go Outdated Show resolved Hide resolved
0xKiwi
0xKiwi previously approved these changes Jun 1, 2020
@0xKiwi
Copy link
Contributor

0xKiwi commented Jun 2, 2020

Tests are failing, I've tried to fix but I have to move to other tasks.

// SetValidatorSpan marshal a validator span into an encoded, flattened array.
func (es EpochStore) SetValidatorSpan(ctx context.Context, idx uint64, newSpan types.Span) error {
if len(es)%spannerEncodedLength != 0 {
return errors.New("wrong data length for min max span byte array")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this can be moved into a NewEpochStore([]byte) function? Seems weird to have the function return an error for something that should be checked on creation.

Would be great to make an epoch_store.go with these functions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added NewEpochStore. kept error in place for extra safety

@0xKiwi 0xKiwi merged commit fd19fd1 into master Jun 2, 2020
@farazdagi farazdagi deleted the new_spanner_db branch August 10, 2020 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants