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

Add iteration support to etcd #52199

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Conversation

rosstimothy
Copy link
Contributor

@rosstimothy rosstimothy commented Feb 14, 2025

Introduces a new Items(context.Context, backend.IterateParams) iter.Seq2[backend.Item, error] to the etcd backend. The core logic for this function was moved from GetRange, and GetRange now defers to calling Iterate and collecting a slice of items.

The motivation for this change is to attempt to reduce the complexity of pagination for consumers of GetRange. While backend.IterateRange and backend.StreamRange do already exists to serve the same purpose, they too have suffered from pagination bugs in the past. Additionally, this aims to unify iteration to using the iter package instead of the various homegrown iteration mechanisms that exist throughout the code base.

There is also one distinct difference to the iteration api: it allows retrieving items within the specified range in ascending or descending key order. While there may not be many use cases for this today, exposing this in the api from it's inception is easier than adding functional options, or IterateAscending/IterateDescending later.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Feb 14, 2025
@rosstimothy rosstimothy force-pushed the tross/backend_iterate_etcd branch from fbff95f to 2d169d0 Compare February 14, 2025 20:44
@rosstimothy
Copy link
Contributor Author

I attempted to determine the performance impact of this change by running a modified version of BenchmarkGetNodes to run against etcd and for the sake of time a single node count.

goos: darwin
goarch: arm64
pkg: github.com/gravitational/teleport/lib/services/local
cpu: Apple M2 Pro
                                       │ master.txt  │            new.txt            │
                                       │   sec/op    │   sec/op     vs base          │
GetNodes/tt(nodes=2000)-12   178.5m ± 2%   177.3m ± 1%  ~ (p=0.739 n=10)

                                       │  master.txt  │               new.txt               │
                                       │     B/op     │     B/op      vs base               │
GetNodes/tt(nodes=2000)-12   11.83Mi ± 4%   12.35Mi ± 2%  +4.37% (p=0.000 n=10)

                                       │ master.txt  │              new.txt               │
                                       │  allocs/op  │  allocs/op   vs base               │
GetNodes/tt(nodes=2000)-12   148.0k ± 0%   148.0k ± 0%  +0.03% (p=0.002 n=10)```

@rosstimothy rosstimothy force-pushed the tross/backend_iterate_etcd branch from 2d169d0 to 54cdcff Compare February 14, 2025 21:15
@rosstimothy rosstimothy marked this pull request as ready for review February 14, 2025 21:37
@github-actions github-actions bot requested a review from GavinFrazar February 14, 2025 21:37
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

If we want to follow https://pkg.go.dev/iter#hdr-Naming_Conventions we should call the method Range or something like that, shouldn't we?

Copy link
Contributor

@fspmarshall fspmarshall left a comment

Choose a reason for hiding this comment

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

LGTM once error handling is improved.

@rosstimothy rosstimothy force-pushed the tross/backend_iterate_etcd branch from b16e09f to 0f9445a Compare February 25, 2025 14:32
@rosstimothy rosstimothy force-pushed the tross/backend_iterate_etcd branch from b9dea9e to b4dd722 Compare February 25, 2025 16:28
Introduces a new
`Items(context.Context, backend.IterateParams) iter.Seq2[backend.Item, error]`
to the etcd backend. The core logic for this function was moved from
GetRange, and GetRange now defers to calling Items and collecting a slice
of items.

The motivation for this change is to attempt to reduce the
complexity of pagination for consumers of GetRange. While
backend.IterateRange and backend.StreamRange do already exists to
serve the same purpose, they too have suffered from pagination bugs
in the past. Additionally, this aims to unify iteration to using
the iter package instead of the various homegrown iteration
mechanisms that exist throughout the code base.

There is also one distinct difference to the iteration api: it
allows retrieving items within the specified range in ascending
or descending key order. While there may not be many use cases for
this today, exposing this in the api from it's inception is easier
than adding functional options, or ItemsAscending/ItemsDescending
later.
@rosstimothy rosstimothy force-pushed the tross/backend_iterate_etcd branch from b4dd722 to e97a438 Compare February 25, 2025 17:36
@rosstimothy rosstimothy added this pull request to the merge queue Feb 25, 2025
Merged via the queue into master with commit 6e9a036 Feb 25, 2025
41 checks passed
@rosstimothy rosstimothy deleted the tross/backend_iterate_etcd branch February 25, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants