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.

Sign up for GitHub

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

Jump to bottom

Pagination fixes #127

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Pagination fixes #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e50bc3
Add support for filtering/sorting by array index
cmurphy Sep 18, 2023
1387552
Allow for filtering/sorting by reserved fields
cmurphy Sep 18, 2023
6ab1b12
Changing wrangler imports to use new function
MbolotSuse Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ require (
github.com/rancher/kubernetes-provider-detector v0.1.5
github.com/rancher/norman v0.0.0-20240326183200-dd207ee11dda
github.com/rancher/remotedialer v0.3.1
github.com/rancher/wrangler/v2 v2.2.0-rc1
github.com/rancher/wrangler/v2 v2.2.0-rc2
github.com/sirupsen/logrus v1.9.3
github.com/stretchr/testify v1.9.0
github.com/urfave/cli v1.22.14
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,8 @@ github.com/rancher/norman v0.0.0-20240326183200-dd207ee11dda h1:3ObSlUUnZRLFZAP9
github.com/rancher/norman v0.0.0-20240326183200-dd207ee11dda/go.mod h1:CsrtakSMYQKWlmjwCaqyazoVVOQBX/JwvZSxIHhWxxk=
github.com/rancher/remotedialer v0.3.1 h1:YQ/UZy3w1CtC8dZvR51/e0K9qNFKiPh5zLFlDghNsBg=
github.com/rancher/remotedialer v0.3.1/go.mod h1:Aa9TrMVN3uRAKIAxio93T/hAyqad+054Luceu7sVmsE=
github.com/rancher/wrangler/v2 v2.2.0-rc1 h1:5T1Bk87tmwLgqVXsfhrQzr7oeMIXxbnIQcD4qwFA47c=
github.com/rancher/wrangler/v2 v2.2.0-rc1/go.mod h1:YtKhKhNDVdm4lybNttbmh1MmR2p123vi6Gik+4MkUEk=
github.com/rancher/wrangler/v2 v2.2.0-rc2 h1:b8HPe21hg2/wbH26wyblSG5XfMYjbL6jQjiYjrXiq08=
github.com/rancher/wrangler/v2 v2.2.0-rc2/go.mod h1:YtKhKhNDVdm4lybNttbmh1MmR2p123vi6Gik+4MkUEk=
github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs=
Expand Down
34 changes: 18 additions & 16 deletions pkg/stores/partition/listprocessor/processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,19 +258,16 @@ func getLimit(apiOp *types.APIRequest) int {
return limit
}

// FilterList accepts a channel of unstructured objects and a slice of filters and returns the filtered list.
// FilterList accepts a slice of unstructured objects and a slice of filters and returns the filtered list.
// Filters are ANDed together.
func FilterList(list <-chan []unstructured.Unstructured, filters []OrFilter) []unstructured.Unstructured {
func FilterList(list []unstructured.Unstructured, filters []OrFilter) []unstructured.Unstructured {
if len(filters) == 0 {
return list
}
Comment on lines +264 to +266
Copy link
Contributor

Choose a reason for hiding this comment

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

Observation: The behavior is slightly different than before because here we're returning the same list (with the same storage behind) whereas before we were returning a new list (different storage).

This doesn't seem to be an issue in this case, and we're doing the same in other places (eg: FilterByProjectsAndNamespaces)

Copy link
Contributor

Choose a reason for hiding this comment

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

Before I think we had a difference in return vs input type, so that wasn't really possible there. Ordinarily I would fix a small inconsistency like this to avoid difficult-to-locate bugs (like someone changing the original list in a go-routine and not realizing that it affected other values down-the-line), but I think there's a performance impact here to consider - copying the items to another list would have a non-zero performance impact. I would also expect the "no filters" case to be the default for the time being, so I'd like to keep this as-is and avoid a costly copy operation.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay as is. I think consistency would be better from my POV. The costly copy was already there, so adding it back wouldn't make rancher perform worse than it already is. We could benchmark it and see how much optimizing this improves the performance in the grand scheme of things but I'd rather do that as a separate issue and as a need basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without benchmark, it's usually difficult to know if an optimization improves or not. For example, the compiler could already be optimizing this part of the code. It's hard to tell without benchmarks.

result := []unstructured.Unstructured{}
for items := range list {
for _, item := range items {
if len(filters) == 0 {
result = append(result, item)
continue
}
if matchesAll(item.Object, filters) {
result = append(result, item)
}
for _, item := range list {
if matchesAll(item.Object, filters) {
result = append(result, item)
}
}
return result
Expand All @@ -281,7 +278,7 @@ func matchesOne(obj map[string]interface{}, filter Filter) bool {
var ok bool
subField := []string{}
for !ok && len(filter.field) > 0 {
objValue, ok = data.GetValue(obj, filter.field...)
objValue, ok = data.GetValueFromAny(obj, filter.field...)
if !ok {
subField = append(subField, filter.field[len(filter.field)-1])
filter.field = filter.field[:len(filter.field)-1]
Expand Down Expand Up @@ -354,11 +351,11 @@ func SortList(list []unstructured.Unstructured, s Sort) []unstructured.Unstructu
return list
}
sort.Slice(list, func(i, j int) bool {
leftPrime := convert.ToString(data.GetValueN(list[i].Object, s.primaryField...))
rightPrime := convert.ToString(data.GetValueN(list[j].Object, s.primaryField...))
leftPrime := getAndConvert(list[i].Object, s.primaryField...)
rightPrime := getAndConvert(list[j].Object, s.primaryField...)
if leftPrime == rightPrime && len(s.secondaryField) > 0 {
leftSecond := convert.ToString(data.GetValueN(list[i].Object, s.secondaryField...))
rightSecond := convert.ToString(data.GetValueN(list[j].Object, s.secondaryField...))
leftSecond := getAndConvert(list[i].Object, s.secondaryField...)
rightSecond := getAndConvert(list[j].Object, s.secondaryField...)
if s.secondaryOrder == ASC {
return leftSecond < rightSecond
}
Expand All @@ -372,6 +369,11 @@ func SortList(list []unstructured.Unstructured, s Sort) []unstructured.Unstructu
return list
}

func getAndConvert(object map[string]interface{}, keys ...string) string {
val, _ := data.GetValueFromAny(object, keys...)
return convert.ToString(val)
}

// PaginateList returns a subset of the result based on the pagination criteria as well as the total number of pages the caller can expect.
func PaginateList(list []unstructured.Unstructured, p Pagination) ([]unstructured.Unstructured, int) {
if p.pageSize <= 0 {
Expand Down
Loading