-
Notifications
You must be signed in to change notification settings - Fork 117
fix(sip): implicitly paginate sip trunk queries to skirt twirp limits #694
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: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import ( | |
| "google.golang.org/protobuf/proto" | ||
|
|
||
| "github.com/livekit/livekit-cli/v2/pkg/util" | ||
| "github.com/livekit/protocol/livekit" | ||
| ) | ||
|
|
||
| const flagRequest = "request" | ||
|
|
@@ -44,6 +45,16 @@ type protoTypeValidator[T any] interface { | |
| Validate() error | ||
| } | ||
|
|
||
| type paginatedType[T any] interface { | ||
| protoType[T] | ||
| GetPage() *livekit.Pagination | ||
| } | ||
|
|
||
| type paginatedResponseType[T any, D any] interface { | ||
| protoType[T] | ||
| GetItems() []*D | ||
| } | ||
|
|
||
| func ReadRequest[T any, P protoType[T]](cmd *cli.Command) (*T, error) { | ||
| return ReadRequestFileOrLiteral[T, P](cmd.String(flagRequest)) | ||
| } | ||
|
|
@@ -99,6 +110,35 @@ func RequestDesc[T any, _ protoType[T]]() string { | |
| return typ + " as JSON file" | ||
| } | ||
|
|
||
| // Repeatedly calls a paginated list function until all items are retrieved, | ||
| // requiring the caller to update the request's pagination parameters as needed. | ||
| func ExhaustivePaginatedList[ | ||
| ReqT any, Req paginatedType[ReqT], | ||
| ResT any, ResD any, Res paginatedResponseType[ResT, ResD], | ||
| ]( | ||
| ctx context.Context, | ||
| req Req, | ||
| list func(context.Context, Req) (Res, error), | ||
| iter func(items []*ResD), | ||
| page *livekit.Pagination, | ||
| ) error { | ||
| for { | ||
| res, err := list(ctx, req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| resultItems := res.GetItems() | ||
| if len(resultItems) > 0 { | ||
| iter(resultItems) | ||
| } | ||
| // List exhausted | ||
| if len(resultItems) < int(page.Limit) { | ||
| break | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
Contributor
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 we need to worry about local memory if the api returns a large amount say thousands of phone numbers ?
Member
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 don't know any modern machine that couldn't handle a few MB or even tens of MB or allocation, and this is a short-lived object. I would suggest we impose limit if and only if we get issues? This worked fine with 22K trunks in testing. |
||
|
|
||
| func createAndPrintFile[T any, P protoTypeValidator[T], R any]( | ||
| ctx context.Context, | ||
| cmd *cli.Command, file string, | ||
|
|
@@ -206,7 +246,7 @@ func listAndPrint[ | |
| ]( | ||
| ctx context.Context, | ||
| cmd *cli.Command, | ||
| getList func(ctx context.Context, req Req) (Resp, error), req Req, | ||
| getList func(context.Context, Req) (Resp, error), req Req, | ||
| header []string, tableRow func(item *T) []string, | ||
| ) error { | ||
| res, err := getList(ctx, req) | ||
|
|
||
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.
We plan to use this for phone numbers
https://github.com/livekit/protocol/blob/main/protobufs/livekit_models.proto#L31
Should we account for this as well ?
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.
Sure, I'll take a look