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

fix test race condition #9

Merged
merged 1 commit into from
Sep 21, 2018
Merged

fix test race condition #9

merged 1 commit into from
Sep 21, 2018

Conversation

Stebalien
Copy link
Member

Also stop exposing functions that don't do what they claim to do:

  • Invalidate does, technically invalidate. However, it's not threadsafe.
  • Rebuild doesn't clear the filter so it's only useful for the initial build.
    It's also not threadsafe.

We can restore these functions later if we need them (but we'll have to change a
few things to make them work properly).

Also adds a Wait function to allow waiting for the bloom filter to finish
building.

fixes #6

Also stop exposing functions that don't do what they claim to do:

* Invalidate does, technically invalidate. However, it's not threadsafe.
* Rebuild doesn't *clear* the filter so it's only useful for the initial build.
  It's also not threadsafe.

We can restore these functions later if we need them (but we'll have to change a
few things to make them work properly).

Also adds a `Wait` function to allow waiting for the bloom filter to finish
building.

fixes #6
case <-ctx.Done():
return ctx.Err()
case <-b.buildChan:
return b.buildErr
Copy link
Member Author

Choose a reason for hiding this comment

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

This way we have a way of getting at these errors.

@@ -102,7 +98,7 @@ func TestHasIsBloomCached(t *testing.T) {
}

if float64(cacheFails)/float64(1000) > float64(0.05) {
t.Fatal("Bloom filter has cache miss rate of more than 5%")
t.Fatalf("Bloom filter has cache miss rate of more than 5%%")
Copy link
Member Author

Choose a reason for hiding this comment

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

Go vet wasn't happy.

// This chan is only used for testing to wait for bloom to enable
rebuildChan chan struct{}
blockstore Blockstore
bloom *bloom.Bloom
Copy link
Member Author

Choose a reason for hiding this comment

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

Put the atomic first because go can be stupid about things like this.

"Total number of requests to bloom cache").Counter(),
buildChan: make(chan struct{}),
}
go func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Might as well do this all in one goroutine.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

@Stebalien Stebalien merged commit c922e24 into master Sep 21, 2018
@Stebalien Stebalien deleted the fix/6 branch September 21, 2018 15:57
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.

TestHasIsBloomCached failures
2 participants