-
Notifications
You must be signed in to change notification settings - Fork 56
Add draft of throttled copy #258
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
f1ac460
80627b2
4e9f68e
0fc9e60
1ccde5b
0702186
dd156c5
56497cc
3560177
681a6f5
eb52eb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| package desync | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestCopy(t *testing.T) { | ||
| src_store_dir := t.TempDir() | ||
| dst_store_dir := t.TempDir() | ||
|
|
||
| src_store, err := NewLocalStore(src_store_dir, StoreOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| dst_store, err := NewLocalStore(dst_store_dir, StoreOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| first_chunk_data := []byte("some data") | ||
| first_chunk := NewChunk(first_chunk_data) | ||
| first_chunk_id := first_chunk.ID() | ||
|
|
||
| src_store.StoreChunk(first_chunk) | ||
| has_the_stored_chunk, _ := src_store.HasChunk(first_chunk_id) | ||
| require.True(t, has_the_stored_chunk) | ||
|
|
||
| chunks := make([]ChunkID, 1) | ||
| chunks[0] = first_chunk_id | ||
|
|
||
| Copy(context.Background(), chunks, src_store, dst_store, 1, NewProgressBar("")) | ||
| require.NoError(t, err) | ||
| has_the_chunk, _ := dst_store.HasChunk(first_chunk_id) | ||
|
|
||
| require.True(t, has_the_chunk) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package desync | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "golang.org/x/time/rate" | ||
| ) | ||
|
|
||
| type ThrottleOptions struct { | ||
| eventRate float64 | ||
| burstRate int | ||
| timeout time.Duration | ||
| immediateOrFail bool | ||
| } | ||
|
|
||
| type RateLimitedLocalStore struct { | ||
|
|
||
| wrappedStore WriteStore | ||
|
|
||
| limiter *rate.Limiter | ||
|
|
||
| options ThrottleOptions | ||
|
|
||
| } | ||
|
|
||
| var RateLimitedExceeded = errors.New("Rate Limit Exceeded") | ||
|
|
||
| func NewRateLimitedLocalStore(s WriteStore, options ThrottleOptions) *RateLimitedLocalStore { | ||
|
|
||
| limiter := rate.NewLimiter(rate.Limit(options.eventRate), options.burstRate) | ||
| return &RateLimitedLocalStore{wrappedStore: s,limiter: limiter, options: options } | ||
| } | ||
|
|
||
| func (s RateLimitedLocalStore) GetChunk(id ChunkID) (*Chunk, error) { | ||
|
|
||
| return s.wrappedStore.GetChunk(id) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you wanted to limit reads as well (and I think we should), it would make sense to first call GetChunk() on the underlying store, then wait after we have the data. That isn't super-precise in terms of limiting since there could be bursts of maxChunkSize * n at startup but over time it should even out.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'll invert the order and add in support for reads. |
||
| } | ||
|
|
||
| func (s RateLimitedLocalStore) HasChunk(id ChunkID) (bool, error) { | ||
|
|
||
|
|
||
| return s.wrappedStore.HasChunk(id) | ||
| } | ||
|
|
||
|
|
||
| func (s RateLimitedLocalStore) StoreChunk(chunk *Chunk) error { | ||
|
|
||
| // This isn't ideal because what I'm really interested is in size over the wire. | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can actually achieve this by using a limit of "bytes per second" (must be equal or larger than max-chunk-size). The here in b, err := chunk.Data()
if err != nil {
return err
}
err = s.limiter.WaitN(ctx, len(b))
if err != nil {
return RateLimitedExceeded
}
return s.wrappedStore.StoreChunk(chunk)It's a little more interesting in As for |
||
| _, err := chunk.Data() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| //size := len(b) | ||
| ctx, cancel:= context.WithTimeout(context.Background(), s.options.timeout) | ||
| defer cancel() | ||
|
|
||
| if s.options.immediateOrFail{ | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you actually need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could go either way. I guess I'd lean towards removal. Related: We could have two paths, one where we use a context with timeout and one without. But that's another code path.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd certainly be nice to have a context on all these functions but that's a major refactoring of the library that should be done in a separate PR. The wait really depends on the rate-limit here, and I don't think it should hang forever. Adding a timeout seems overly complex and I'd remove that. It might be possible to calculate some upper limit based on the rate-limit used and the max chunk size perhaps.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To confirm my understanding:
I'm ultimately only concerned with throttling cache operations against my local store, so I think that's fine for me. However, I haven't looked at the serving code much, is there a chance that this affects HTTP handling?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, no custom timeout. This can be implemented some day when contexts are added to all store operations. This shouldn't affect HTTP stores at all, since we're not really limiting the data-rate, but the speed at which whole chunks can be read/stored. So if the rate-limiter allows one chunk to be accessed, that will be transferred at max speed which means the HTTP timeouts apply like normal. Chunk downloading is limited to the rate-limit on average, not for individual chunks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test with the HttpHandler. There is a way to reproduce an error.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's just part of the stacktrace. Is there a repro on your branch? What test can I run to see it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I went back to look I couldn't repro. |
||
| if !s.limiter.AllowN(time.Now(),1){ | ||
| err = errors.New("Unable to immediately store") | ||
| } | ||
| } else{ | ||
| err = s.limiter.WaitN(ctx,1) | ||
| } | ||
|
|
||
| if err != nil { | ||
|
|
||
| fmt.Println("Rate limit context error:", err) | ||
| return RateLimitedExceeded | ||
| } | ||
|
|
||
| return s.wrappedStore.StoreChunk(chunk) | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| package desync | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/rand" | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "golang.org/x/time/rate" | ||
| ) | ||
|
|
||
|
|
||
| func NewTestRateLimitedLocalStore(t *testing.T, eventRate float64, burstRate int, timeout time.Duration, immediateOrFail bool) *RateLimitedLocalStore{ | ||
|
|
||
| src_store_dir := t.TempDir() | ||
| src_store, err := NewLocalStore(src_store_dir, StoreOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| throttleOptions := ThrottleOptions{eventRate,burstRate,timeout,immediateOrFail} | ||
| store :=NewRateLimitedLocalStore(src_store, throttleOptions) | ||
| require.Equal(t,store.options.burstRate,burstRate ) | ||
| return store | ||
|
|
||
| } | ||
|
|
||
| func random_data() ([]byte,error){ | ||
|
|
||
| b := make([]byte, 16) | ||
| _, err := rand.Read(b) | ||
| if err != nil { | ||
| fmt.Println("Error: ", err) | ||
| return b,err | ||
| } | ||
|
|
||
| return b,nil | ||
| } | ||
|
|
||
|
|
||
| func storeLoop(t *testing.T, max int, chunk_ids []ChunkID, store RateLimitedLocalStore){ | ||
|
|
||
| for i := 0; i < max; i++ { | ||
|
|
||
| data,err := random_data() | ||
| require.NoError(t,err) | ||
| chunk := NewChunk(data) | ||
| chunk_ids[i] = chunk.ID() | ||
| err = store.StoreChunk(chunk) | ||
| require.Nil(t,err) | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
|
||
| func chunkCheck(t *testing.T, max int, chunk_ids []ChunkID, store RateLimitedLocalStore) { | ||
| for i := 0; i < max; i++ { | ||
|
|
||
| has,err := store.HasChunk(chunk_ids[i]) | ||
| require.Nil(t,err) | ||
| require.True(t,has) | ||
|
|
||
| } | ||
| } | ||
|
|
||
| func TestLimiter(t *testing.T){ | ||
|
|
||
| // This is testing the framework. So not great, but I needed to get my head around what the relation | ||
| // between events and burst do | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*1000) | ||
| defer cancel() | ||
|
|
||
| limiter := rate.NewLimiter(rate.Limit(50), 50) | ||
| err := limiter.WaitN(ctx,1) | ||
| require.Nil(t, err) | ||
|
|
||
| limiter = rate.NewLimiter(rate.Limit(2), 1) | ||
| _ = limiter.WaitN(ctx,1) | ||
| require.Nil(t, err) | ||
| err = limiter.WaitN(ctx,1) | ||
| require.Nil(t, err) | ||
|
|
||
| limiter = rate.NewLimiter(rate.Limit(1), 1) | ||
| _ = limiter.WaitN(ctx,1) | ||
| require.Nil(t, err) | ||
| err = limiter.WaitN(ctx,1) | ||
| require.NotNil(t, err) | ||
|
|
||
|
|
||
|
|
||
|
|
||
| } | ||
|
|
||
| func TestCopyWithNoLimit(t *testing.T) { | ||
|
|
||
|
|
||
| throttledStore := NewTestRateLimitedLocalStore(t,1,1,time.Second*60, false) | ||
| chunk_data := []byte("different datas") | ||
| chunk := NewChunk(chunk_data) | ||
|
|
||
|
|
||
| start := time.Now() | ||
| // We start with 1 token in the bucket and replenish at 1 token per second | ||
| // This should take ~10 seconds. | ||
| // We test it takes 8s to guard against flakiness | ||
| for i := 0; i < 10; i++ { | ||
| err := throttledStore.StoreChunk(chunk) | ||
| // This test will eventually fail when I get deadlines enabled | ||
| require.Nil(t,err) | ||
| } | ||
| finish := time.Now() | ||
|
|
||
| require.True(t, finish.Sub(start).Seconds() > 8) | ||
| } | ||
|
|
||
|
|
||
| func TestForAFullBucketNoWait(t *testing.T) { | ||
|
|
||
| chunk_count := 10 | ||
| // Bucket has initial size chunk_count | ||
| throttledStore := NewTestRateLimitedLocalStore(t,1,chunk_count + 1,time.Second*60, false) | ||
|
|
||
| chunk_ids := make([]ChunkID, chunk_count) | ||
| start := time.Now() | ||
| // The bucket is full, we shouldn't have to wait | ||
| storeLoop(t,chunk_count,chunk_ids,*throttledStore) | ||
| finish := time.Now() | ||
| require.True(t, finish.Sub(start).Seconds() < 2) | ||
| chunkCheck(t,chunk_count,chunk_ids,*throttledStore) | ||
| } | ||
|
|
||
| func TestForAFastReplenishmentRateLittleWait(t *testing.T) { | ||
|
|
||
| chunk_count := 10 | ||
| // Bucket only has one token, but we replenish chunk_count tokens every second | ||
| throttledStore := NewTestRateLimitedLocalStore(t,float64( chunk_count + 1),1,time.Second*60,false) | ||
|
|
||
| start := time.Now() | ||
|
|
||
|
|
||
| chunk_ids := make([]ChunkID, chunk_count) | ||
| storeLoop(t,chunk_count,chunk_ids,*throttledStore) | ||
|
|
||
| finish := time.Now() | ||
| require.True(t, finish.Sub(start).Seconds() < 2) | ||
| chunkCheck(t,chunk_count,chunk_ids,*throttledStore) | ||
|
|
||
|
|
||
| } | ||
|
|
||
| func TestTimeout(t *testing.T) { | ||
|
|
||
| // Bucket only has one token, and we replenish very slowly. We timeout, so second invocation will fail | ||
| throttledStore := NewTestRateLimitedLocalStore(t,float64(1) /100,1,time.Millisecond*1000, false) | ||
|
|
||
|
|
||
|
|
||
| data,err := random_data() | ||
| require.NoError(t,err) | ||
| chunk := NewChunk(data) | ||
| err = throttledStore.StoreChunk(chunk) | ||
| require.Nil(t,err) | ||
| err = throttledStore.StoreChunk(chunk) | ||
| require.NotNil(t,err) | ||
| require.True(t, errors.Is(err,RateLimitedExceeded)) | ||
| } | ||
|
|
||
| func TestNoTimeout(t *testing.T) { | ||
| chunk_count := 10 | ||
| // Bucket only has one token, replenish 1 per second. Timeout is 11 seconds. | ||
| throttledStore := NewTestRateLimitedLocalStore(t,1,1,time.Second*11, false) | ||
|
|
||
|
|
||
| chunk_ids := make([]ChunkID, chunk_count) | ||
| storeLoop(t,chunk_count,chunk_ids,*throttledStore) | ||
| } | ||
|
|
||
| func TestImmediateOrFail(t *testing.T) { | ||
|
|
||
| // Bucket only has one token, and we replenish very slowly. Second invocation will fail | ||
| throttledStore := NewTestRateLimitedLocalStore(t,float64(1) /100,1,time.Second*60, true) | ||
|
|
||
|
|
||
|
|
||
| data,err := random_data() | ||
| require.NoError(t,err) | ||
| chunk := NewChunk(data) | ||
|
|
||
| err = throttledStore.StoreChunk(chunk) | ||
| require.Nil(t,err) | ||
|
|
||
| err = throttledStore.StoreChunk(chunk) | ||
| require.NotNil(t,err) | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the
Localin the name. This could limit any kind of store.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I'll fix. I think I was originally wrapping a local store only.