Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Find random corruptions #1009

Merged
merged 8 commits into from
Aug 24, 2018
Merged

Find random corruptions #1009

merged 8 commits into from
Aug 24, 2018

Conversation

replay
Copy link
Contributor

@replay replay commented Aug 21, 2018

Not for merging

Just wanted to show you that I believe that there are probably no ways left to corrupt the cache, as long as the first parameter of Add()/AddRange() (the previous ts) is not above 0 and wrong.
This randomly adds/deletes stuff from the cache and then keeps checking if it can find a corruption.
I've run that a bunch of times, that's just from one run:

mst@mst-nb1:~/documents/code/go/src/github.com/grafana/metrictank$ go test github.com/grafana/metrictank/mdata/cache -run TestCorruptionCase2 -test.v -c
mst@mst-nb1:~/documents/code/go/src/github.com/grafana/metrictank$ time ./cache.test -test.run TestCorruptionCase2
adds: 33343523 addRanges: 33331415 dels:33325062
PASS

real	15m53.706s
user	16m11.340s
sys	0m3.484s

@replay replay requested a review from Dieterbe August 21, 2018 20:10
since ~1/3 of the time, we add ranges which typically have a length of tens
of chunks, and ~1/3 of the time we add single chunks,
the ~1/3 of the time single chunk deletes can't make much impact and i
imagine the cache would quickly fill up and we don't exercise the
delete path very well.

This balances out the average deletions vs the average
additions and should thus exercise all different code paths more evenly.

Also, update stats so we can compare both number of operations but
also number of total chunk adds/deletes, instead of having those masked
via the range  operations.
@Dieterbe
Copy link
Contributor

Mauro that's a really interesting approach.
I think we can include this in the standard test suite if we just reduce the runtime (num iterations)
please have a look at the commits I added.

I think one thing we should add to this is track exactly which chunks we should have at each point in time
(perhaps with a [100]bool where each operation adjusts the bools as needed), and confirm that the CCM matches.

// end >= start and both numbers drawn from [min, max)
func getRandomRange(min, max int) (int, int) {
start := getRandomNumber(min, max)
end := getRandomNumber(start, max)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i just realized that this randomness is skewed to the end of the range because, assuming the distribution of start across min - max is even, and considering that end always must be >= start, then the middle of the resulting range returned from getRandomRange() at average will above the middle of the range min-max.
that's not really a problem, just a note

Copy link
Contributor

Choose a reason for hiding this comment

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

we could keep a counter, and every other time, draw an end first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, we could also just get two random numbers and return the lower one as start and the higher as end

Copy link
Contributor

Choose a reason for hiding this comment

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

oh that's elegant I like that.


for i := 0; i < iterations; i++ {
// 0 = Add
// 1 = AddRange
// 2 = Del
action := getRandomNumber(0, 3)
// 2 = Del range (via multi del cals)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be 3

@replay
Copy link
Contributor Author

replay commented Aug 22, 2018

your changes LGTM @Dieterbe
I've pushed another small commit

@Dieterbe
Copy link
Contributor

@replay how does the latest commit look ? I think it's good to merge now

@replay
Copy link
Contributor Author

replay commented Aug 24, 2018

that's nice @Dieterbe , makes sense

@Dieterbe Dieterbe merged commit ff9c8e1 into master Aug 24, 2018
@Dieterbe Dieterbe deleted the find_random_corruptions branch September 18, 2018 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants