-
Notifications
You must be signed in to change notification settings - Fork 514
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
Hedgin Bets and Requests #750
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
029f01e
hedged client
joe-elliott 85e9059
edited vendored files!
joe-elliott e0d6a9d
2 seconds?
joe-elliott fd6ee22
don't cancel
joe-elliott 411df7c
Upgraded to 0.4.0
joe-elliott 2267a93
Added config and docs
joe-elliott c6c370a
Cleaned up bucket creation logic
joe-elliott a92cdc6
Added to s3
joe-elliott 8a176aa
changelog
joe-elliott 1e3eead
Merge branch 'main' into hedgin
joe-elliott 6c46892
go mod tidy
joe-elliott e8bc900
go mod vendor
joe-elliott c946d0f
be a better developer
joe-elliott 7fdb6e4
Added doc details
joe-elliott 3724958
s3 tests?
joe-elliott 2c8ca32
gcs tests?
joe-elliott a857f21
azure support
joe-elliott 4fed7c6
lint
joe-elliott 5a9a2f1
vendor
joe-elliott aab203e
fixed races
joe-elliott c831bff
review
joe-elliott File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| package azure | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/uuid" | ||
| "github.com/grafana/tempo/tempodb/backend" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestHedge(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| returnIn time.Duration | ||
| hedgeAt time.Duration | ||
| expectedHedgedRequests int32 | ||
| }{ | ||
| { | ||
| name: "hedge disabled", | ||
| expectedHedgedRequests: 1, | ||
| }, | ||
| { | ||
| name: "hedge enabled doesn't hit", | ||
| hedgeAt: time.Hour, | ||
| expectedHedgedRequests: 1, | ||
| }, | ||
| { | ||
| name: "hedge enabled and hits", | ||
| hedgeAt: time.Millisecond, | ||
| returnIn: 100 * time.Millisecond, | ||
| expectedHedgedRequests: 2, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| count := int32(0) | ||
| server := fakeServer(t, tc.returnIn, &count) | ||
|
|
||
| r, w, _, err := New(&Config{ | ||
| MaxBuffers: 3, | ||
| BufferSize: 1000, | ||
| ContainerName: "blerg", | ||
| Endpoint: server.URL[7:], // [7:] -> strip http://, | ||
| HedgeRequestsAt: tc.hedgeAt, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| // the first call on each client initiates an extra http request | ||
| // clearing that here | ||
| _, _ = r.Read(ctx, "object", uuid.New(), "tenant") | ||
| time.Sleep(tc.returnIn) | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| // calls that should hedge | ||
| _, _ = r.Read(ctx, "object", uuid.New(), "tenant") | ||
| time.Sleep(tc.returnIn) | ||
| assert.Equal(t, tc.expectedHedgedRequests*2, atomic.LoadInt32(&count)) // *2 b/c reads execute a HEAD and GET | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| // this panics with the garbage test setup. todo: make it not panic | ||
| // _ = r.ReadRange(ctx, "object", uuid.New(), "tenant", 10, make([]byte, 100)) | ||
| // time.Sleep(tc.returnIn) | ||
| // assert.Equal(t, tc.expectedHedgedRequests, atomic.LoadInt32(&count)) | ||
| // atomic.StoreInt32(&count, 0) | ||
|
|
||
| _, _ = r.BlockMeta(ctx, uuid.New(), "tenant") // *2 b/c reads execute a HEAD and GET | ||
| time.Sleep(tc.returnIn) | ||
| assert.Equal(t, tc.expectedHedgedRequests*2, atomic.LoadInt32(&count)) | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| // calls that should not hedge | ||
| _, _ = r.Tenants(ctx) | ||
| assert.Equal(t, int32(1), atomic.LoadInt32(&count)) | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| _, _ = r.Blocks(ctx, "tenant") | ||
| assert.Equal(t, int32(1), atomic.LoadInt32(&count)) | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| _ = w.Write(ctx, "object", uuid.New(), "tenant", make([]byte, 10)) | ||
| assert.Equal(t, int32(1), atomic.LoadInt32(&count)) | ||
| atomic.StoreInt32(&count, 0) | ||
|
|
||
| _ = w.WriteBlockMeta(ctx, &backend.BlockMeta{}) | ||
| assert.Equal(t, int32(1), atomic.LoadInt32(&count)) | ||
| atomic.StoreInt32(&count, 0) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func fakeServer(t *testing.T, returnIn time.Duration, counter *int32) *httptest.Server { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| time.Sleep(returnIn) | ||
|
|
||
| atomic.AddInt32(counter, 1) | ||
| _, _ = w.Write([]byte(`{}`)) | ||
| })) | ||
| t.Cleanup(server.Close) | ||
|
|
||
| return server | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| package gcs | ||
|
|
||
| import "time" | ||
|
|
||
| type Config struct { | ||
| BucketName string `yaml:"bucket_name"` | ||
| ChunkBufferSize int `yaml:"chunk_buffer_size"` | ||
| Endpoint string `yaml:"endpoint"` | ||
| Insecure bool `yaml:"insecure"` | ||
| BucketName string `yaml:"bucket_name"` | ||
| ChunkBufferSize int `yaml:"chunk_buffer_size"` | ||
| Endpoint string `yaml:"endpoint"` | ||
| Insecure bool `yaml:"insecure"` | ||
| HedgeRequestsAt time.Duration `yaml:"hedge_requests_at"` | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not specific to this PR: I noticed this config uses kebab-case instead of snake_case like the other configs. Is this intentional? Is this just some debt we can't get rid of anymore now?
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.
yeah, i noticed this for the first time adding this config option. i think everything is snake except for this azure config. we should consider a breaking change PR where we move azure to the same standard as everything else.