-
Notifications
You must be signed in to change notification settings - Fork 0
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
support shard-based routing #89
Merged
Merged
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
7800470
setup env & testing for shard routing
pirtleshell 08d88a8
rename HeightShardingProxies -> PruningOrDefaultProxies
pirtleshell 7774272
implement ShardProxies for shard routing
pirtleshell 5a9f4c4
update failing test for shard routing
pirtleshell 3c53de9
add tests for shard backend responses
pirtleshell 1afa8c8
add unit tests for shard routing proxies
pirtleshell bbca4de
route "earliest" to first shard
pirtleshell e737fa0
configure shard routing in CI tests
pirtleshell fb04d8b
document shard-based routing
pirtleshell b468bb9
make shards inclusive of end block
pirtleshell 1955b36
fix comment typo
pirtleshell 00d98e4
add more validation to shard backend url map
pirtleshell 8eaa1bc
fix doc typo
pirtleshell dafa109
actually validate shard backend url map
pirtleshell 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package config | ||
|
||
import ( | ||
"net/url" | ||
"sort" | ||
) | ||
|
||
// IntervalURLMap stores URLs associated with a range of numbers. | ||
// The intervals are defined by their endpoints and must not overlap. | ||
// The intervals are exclusive of the endpoints. | ||
type IntervalURLMap struct { | ||
UrlByEndHeight map[uint64]*url.URL | ||
endpoints []uint64 | ||
} | ||
|
||
// NewIntervalURLMap creates a new IntervalMap from a map of interval endpoint => url. | ||
// The intervals are exclusive of their endpoint. | ||
// ie. if the lowest value endpoint in the map is 10, the interval is for all numbers 1 through 9. | ||
func NewIntervalURLMap(urlByEndHeight map[uint64]*url.URL) IntervalURLMap { | ||
endpoints := make([]uint64, 0, len(urlByEndHeight)) | ||
for e := range urlByEndHeight { | ||
endpoints = append(endpoints, e) | ||
} | ||
sort.Slice(endpoints, func(i, j int) bool { return endpoints[i] < endpoints[j] }) | ||
|
||
return IntervalURLMap{ | ||
UrlByEndHeight: urlByEndHeight, | ||
endpoints: endpoints, | ||
} | ||
} | ||
|
||
// Lookup finds the value associated with the interval containing the number, if it exists. | ||
func (im *IntervalURLMap) Lookup(num uint64) (*url.URL, uint64, bool) { | ||
i := sort.Search(len(im.endpoints), func(i int) bool { return im.endpoints[i] > num }) | ||
|
||
if i < len(im.endpoints) && num < im.endpoints[i] { | ||
return im.UrlByEndHeight[im.endpoints[i]], im.endpoints[i], true | ||
} | ||
|
||
return nil, 0, false | ||
} |
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,56 @@ | ||
package config_test | ||
|
||
import ( | ||
"fmt" | ||
"net/url" | ||
"testing" | ||
|
||
"github.com/kava-labs/kava-proxy-service/config" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func mustUrl(s string) *url.URL { | ||
u, err := url.Parse(s) | ||
if err != nil { | ||
panic(fmt.Sprintf("failed to parse url %s: %s", s, err)) | ||
} | ||
return u | ||
} | ||
|
||
func TestUnitTestIntervalMap(t *testing.T) { | ||
valueByEndpoint := map[uint64]*url.URL{ | ||
10: mustUrl("A"), | ||
20: mustUrl("B"), | ||
100: mustUrl("C"), | ||
} | ||
intervalmap := config.NewIntervalURLMap(valueByEndpoint) | ||
|
||
testCases := []struct { | ||
value uint64 | ||
expectFound bool | ||
expectEndHeight uint64 | ||
expectResult string | ||
}{ | ||
{1, true, 10, "A"}, | ||
{9, true, 10, "A"}, | ||
{10, true, 20, "B"}, | ||
{15, true, 20, "B"}, | ||
{20, true, 100, "C"}, | ||
{75, true, 100, "C"}, | ||
{100, false, 0, ""}, | ||
{300, false, 0, ""}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(fmt.Sprintf("Lookup(%d)", tc.value), func(t *testing.T) { | ||
result, endHeight, found := intervalmap.Lookup(tc.value) | ||
require.Equal(t, tc.expectFound, found) | ||
require.Equal(t, tc.expectEndHeight, endHeight) | ||
if tc.expectResult == "" { | ||
require.Nil(t, result) | ||
} else { | ||
require.Equal(t, tc.expectResult, result.String()) | ||
} | ||
}) | ||
} | ||
} |
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
Oops, something went wrong.
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.
to enforce line 9 comments of
// The intervals are defined by their endpoints and must not overlap.
might be handy to validate the map is discontinuous and return an error in this new functionThere 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.
sorry, i don't follow. the intervals are only defined by one point (the endpoint), so the only way you couldn't create a valid interval is if you set the same endpoint to two different URLs.
are you suggesting i panic if they set the same endpoint multiple times for the same host?
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.
thanks though! drawing your attention to this made me realize i was still on the branch that had exclusive endpoints. because of some simplifications i made to the shard command, it was actually easier to make them inclusive of the end block. just updated the code & tests to reflect that change (that i already had in the docs 😅)
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.
basically I'm saying there are properties we are expecting this type to have that will effect how the service functions and is configured by hands by human, those two things in conjunction make me think it's worth adding some validation that the parsed config is not only syntactically correct but semantically correct - whether that be inclusive / exclusive or unique (fwiw I was referring to testing the inclusive property in terms that the supplied interval map doesn't have any gaps, e.g. config for a shard for blocks 10-50, and for shard 100-latest since this is something configured by a human operator and human operators err but the case of duplicate endpoints is also a good edge case to validate)
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.
gotcha. at this point the value has already been parsed into a valid map. the encoding is such that you can't define non-continuous ranges of shards (only endblocks are defined. the first shard starts at height 1, and then each shard covers from the previous shard's endBlock + 1 to its defined end block).
i've added some more validations to the parsing of the env value to hopefully catch misunderstandings or misconfigurations: