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

chore(*) introduce PaginationStore #1144

Merged
merged 13 commits into from
Nov 16, 2020
Merged

chore(*) introduce PaginationStore #1144

merged 13 commits into from
Nov 16, 2020

Conversation

nickolaev
Copy link
Contributor

Summary

Introduce the PaginationStore and delegate all pagination to be done there instead of implementation specific.

Nikolay Nikolaev added 6 commits November 12, 2020 11:47
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev marked this pull request as ready for review November 12, 2020 09:53
@nickolaev nickolaev requested a review from a team as a code owner November 12, 2020 09:53
pkg/core/resources/store/pagination_store.go Outdated Show resolved Hide resolved
pkg/core/resources/store/pagination_store.go Outdated Show resolved Hide resolved
pkg/core/resources/store/pagination_store.go Show resolved Hide resolved
pkg/core/resources/store/options.go Outdated Show resolved Hide resolved
pkg/core/resources/store/pagination_store.go Outdated Show resolved Hide resolved
pkg/core/resources/store/pagination_store.go Outdated Show resolved Hide resolved
pkg/core/resources/store/pagination_store.go Outdated Show resolved Hide resolved
Nikolay Nikolaev added 2 commits November 12, 2020 20:14
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@bloqhead
Copy link

bloqhead commented Nov 12, 2020

I'm trying this out and this is the query:

http://localhost:5681/dataplanes+insights?gateway=true&size=12

I still receive this error though:

{
  "title": "Could not retrieve dataplane overviews",
  "details": "Pagination and filtering at the same time is not supported"
}

Please let me know if I'm doing something wrong 😅.

Should this change work for Dataplane Insights also? My scenario is that I want to fetch only Gateway Dataplanes but I also want to reduce the size for pagination to 12 items per page. I would then do the same for Ingress and Standard (neither Ingress nor Gateway) Dataplanes.

I'm trying this in Universal on macOS. Thanks for the work you're putting in on this! It will make the GUI much more useful to the end user and give us a more robust HTTP API.


Edit: This commit fixes the issue! Thanks!

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

// The Pagination Store is handling only the pagination functionality in the List.
// This is an in-memory operation and offloads this from the persistent stores (k8s, postgres etc.)
// which makes their implementation more simple. The in-memory filtering has been tested with 10,000
Copy link
Contributor

Choose a reason for hiding this comment

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

which makes their implementation more simple

that's not the dealbreaker. Dealbreaker was that it's not even possible to implement this on K8S. We could have implemented in memory pagination only on Kubernetes and native on Postgres, but it's still difficult because our DB schema in Postgres.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I don't see this to be an incorrect statement. We are not explaining what is and what is not a dealbreaker in the code IMO. The point is to leave a note here for our future selves what actually is going on here. When the next persistent storage gets introduced we'll not even consider paging, we'll just do the simple "get all" version and let this implementation deal with paging. Which is actually the point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like to write comments that answer why is this (and what was the alternative) not what is this.

The point is to leave a note here for our future selves what actually is going on here

Exactly. So imagine there is a new developer on the team and we have performance problems with pagination. He takes this task and finds this code and reads "which makes their implementation more simple" and "proved to be fast enough, although not that efficient". So I see there was a compromise, we thought it was fast enough but it isn't now. But what was the alternative? I'd like to see in the comment that

  • There is no filtering + pagination on the native K8S database, so you have to do this in memory anyways.
  • On Postgres, we keep the object in a column as a string. We would have to use JSON column type and convert it to native SQL queries.

the next persistent storage gets introduced we'll not even consider paging (...) Which is actually the point

IMHO the main point was to get Kube and Postgres to work

// This is an in-memory operation and offloads this from the persistent stores (k8s, postgres etc.)
// which makes their implementation more simple. The in-memory filtering has been tested with 10,000
// Dataplanes and proved to be fast enough, although not that efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: delete empty line

pkg/core/resources/store/pagination_store.go Show resolved Hide resolved
}

filteredItems := filteredList.GetItems()
lenFilteresItems := len(filteredItems)
Copy link
Contributor

Choose a reason for hiding this comment

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

filteres typo

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

// The Pagination Store is handling only the pagination functionality in the List.
// This is an in-memory operation and offloads this from the persistent stores (k8s, postgres etc.)
// which makes their implementation more simple. The in-memory filtering has been tested with 10,000
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to write comments that answer why is this (and what was the alternative) not what is this.

The point is to leave a note here for our future selves what actually is going on here

Exactly. So imagine there is a new developer on the team and we have performance problems with pagination. He takes this task and finds this code and reads "which makes their implementation more simple" and "proved to be fast enough, although not that efficient". So I see there was a compromise, we thought it was fast enough but it isn't now. But what was the alternative? I'd like to see in the comment that

  • There is no filtering + pagination on the native K8S database, so you have to do this in memory anyways.
  • On Postgres, we keep the object in a column as a string. We would have to use JSON column type and convert it to native SQL queries.

the next persistent storage gets introduced we'll not even consider paging (...) Which is actually the point

IMHO the main point was to get Kube and Postgres to work

Nikolay Nikolaev added 3 commits November 16, 2020 14:41
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev merged commit 9957ec8 into master Nov 16, 2020
@nickolaev nickolaev deleted the chore/dp_pagination branch November 16, 2020 13:41
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.

None yet

3 participants