Skip to content
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

Use built-in pagination instead of loops. #121

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Sep 2, 2018

Now that the aws sdk has built-in pagination methods, we don't have to write loops and track pagination tokens by hand.

@jmcarp
Copy link
Contributor Author

jmcarp commented Sep 15, 2018

Ping @nickatsegment. This won't change behavior but makes pagination a little simpler to reason about. Plus, we're already using built-in pagination from the sdk elsewhere in the code.

@nickatsegment
Copy link
Contributor

Sounds good to me. I'll give the other core guys a day to comment; I'm not as familiar with the AWS API

@nickatsegment
Copy link
Contributor

I ran the backend benchmarks against master and this PR with

master

$ aws-okta exec dev -- go test -v -run TestSSMConcurrency ./store/... -args -benchmark
=== RUN   TestSSMConcurrency
--- PASS: TestSSMConcurrency (57.53s)
    backendbenchmarks_test.go:55: Concurrently started 1 services in 71.781476ms
    backendbenchmarks_test.go:55: Concurrently started 10 services in 88.639232ms
    backendbenchmarks_test.go:55: Concurrently started 500 services in 25.108277943s
    backendbenchmarks_test.go:55: Concurrently started 1000 services in 29.871719893s
PASS
ok  	github.com/segmentio/chamber/store	57.562s

jmcarp:aws-paginators

go test -v -run TestSSMConcurrency ./store/... -args -benchmark
=== RUN   TestSSMConcurrency
--- PASS: TestSSMConcurrency (47.05s)
    backendbenchmarks_test.go:55: Concurrently started 1 services in 86.868467ms
    backendbenchmarks_test.go:55: Concurrently started 10 services in 89.982798ms
    backendbenchmarks_test.go:55: Concurrently started 500 services in 14.122713244s
    backendbenchmarks_test.go:55: Concurrently started 1000 services in 30.471584343s
PASS
ok  	github.com/segmentio/chamber/store	47.085s

I don't think this test is especially trustworthy (could easily be affected by rate limits from other sources), but it looks roughly the same.

Copy link
Contributor

@systemizer systemizer left a comment

Choose a reason for hiding this comment

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

Didn't know aws had these builtin paginators. Makes sense to me! tested locally and working as intended. thanks again for the contribution @jmcarp !

@nickatsegment nickatsegment merged commit 7b5f2b8 into segmentio:master Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants