Use Kubernetes Pagination for Lists

[alecmerdler]

Description

Modifies the watchK8sList Redux action to use basic k8s pagination to incrementally fetch the full list of k8s objects (set to ?limit=250). This achieves a reduction in the perceived load time for users with huge cluster workloads.

Note: Still needs react-virtualized components to address actual view performance.

Screenshots

Incremental loading (set limit=20 for demo purposes):

Addresses https://jira.coreos.com/browse/CONSOLE-377

[alecmerdler]

Not sure if limit should be passed as a prop or always used by default.

[alecmerdler]

Open Questions:

• Do we always use ?limit=100 for every list, or is it passed as a prop?
  • prefer default behavior for all Firehoses
  • prefer constant incremental loading using ?limit= vs 1 small request using ?limit= + the rest
• Which Redux actions do we dispatch?
  • initialLoad Redux event
• List items get re-ordered as they are fetched because they are alphabetized
  • punt to UX
• Text filtering as new results are loaded
  • punt to UX

[jcaianirh]

@openshift/team-ux-review

this could use some UX weigh-in. paginating requests means that a user will see say.... 50 pods in a list, then as they're about to click on one pod, the next XHR comes back and 50 more pods are inserted in the list and they click on the wrong link and are frustrated
alec, amruta, & I discussed possible solutions, but we didn't come across anything that seemed obviously correct

[serenamarie125]

I'd suggest that the page size is settable by the user. We have a pattern for this in PatternFly today, although there is no suggested "sizes", reason being that the page size likely differs based on the product, technology and performance issues. I would suggest that we allow the user to choose between 25/50/100. @alecmerdler @jcaianirh thoughts?

FYI @beanh66 @cshinn

[alecmerdler]

I'd suggest that the page size is settable by the user

@serenamarie125 The question is not about page size, as this PR does not introduce client-side pagination. We strongly advocate for keeping list views a single page with incrementally loading data:

• Reduces complexity of our already complex list code
• Supports the primary use-case (Go to list, type in text-filter, click desired item)
• Causes more confusion: pages would be added as more data loads in (can't know the size of the list)
  We need a pattern that captures the user's current intent (searching/filtering) while supporting items being added to the list.

[serenamarie125]

@alecmerdler i'm sorry, i misunderstood. Let's see if i get this right - you are incrementally loading the page ? What order is the information in? Can you request that it be ordered based on the current sort order of the page ?

[kans]

I tried this once a while ago, and it didn't work. I must have used Map for my test instead offromJS - the former only works if the nested objects are ===. Nice.

[kans]

What do you think about a higher number? A 10K cluster would require 100 requests to finish loading. For Pods at least, we show the current count which would tick up ... for a while. It looks like pods run about 7KB per item - nodes are closer to 12. A limit of 250 would land at ~2MB per response.

[kans]

A couple of things:

1. It looks like incrementallyLoad will run to completion even if we drop the watch (stopK8sWatch). I didn't look too closely, but we may even continue to start up the rest of the machinery (this could be an existing race condition). We should probably check REF_COUNTS[id] on every iteration and throw() and do magic in the catch.

2. I can imagine incrementallyLoading never finishing on a large cluster with a sad internet. The behavior in that case would be we start loading the list, a XHR drops, then we throw away the data and start over. What do you think about adding a try/catch here to retry any given continueToken N times?

[alecmerdler]

I added some basic retry logic (attempts <= 3) and a naïve attempt to cancel the incremental fetch if a stopK8sWatch is dispatched. I'm not sure the best way to catch the errors I throw, any suggestions @kans?

[kans]

Errors should fall into the error handler on the then - (second function so we don't accidentally catch rendering errors) on 219. It needs to check for ¿both? refcounts and the strings you put in the errors or you could subclass Error and check for that for more flexibility/clarity.

BTW, I don't think throwing strings is cromulent anymore.

[alecmerdler]

@kans I'm having trouble getting an intuition around these edge cases; would you be okay with merging what we have here and making follow-up PRs to address those? I plan to improve our load-testing tools (ServiceWorker, etc) once this is merged.

[kans]

I think the worst case behavior would be that we create WS that live forever. To fix the race, we'd need to check for REF_COUNTS[id] in three places (and throw a special Error):

1. before #L160
2. Before we create the WS on #L180.
3. A new function in the promise.all because pollAndWatch is called recursively for error handling.

We catch all errors on #L213. If the error is special error, I think we just need to exit.

I'd be OK deferring this work to an immediate follow up since we are already subject to the same race.

[kans]

@serenamarie125

Since I don't see a reply:

Let's see if i get this right - you are incrementally loading the page ?

affirmative.

What order is the information in?

Whatever order the API server returns - I'm not exactly sure what it is, but it certainly isn't what we want.

Can you request that it be ordered based on the current sort order of the page?

No. Lists are sorted and filtered client side because of limited functionality in the API.

[ggreer]

It would be nice to check REF_COUNTS[id] in here and abort the loading if it's not > 0. That way we're not trying to load data long after a component has been unmounted.

[kans]

see the hidden discussion above

[ggreer]

I see. I'm also OK with fixing this in a follow-up PR.

[alecmerdler]

Okay, I think we should address it separately because it is an existing problem and not a regression from this PR. I made two TODO comments to remind us.

[ggreer]

Don't we want to dispatch loaded with all the items, not just the ones in the last response? In my experimentation, this causes the list to only show the last bit of data it fetched. eg: set paginationLimit to 10 and try to load all pods. Here's a video of it happening: https://i.imgur.com/NQKiEqL.gifv

[kans]

Good catch!, but we'd need to change the behavior of the reducer to do that. Always dispatching loaded first would probably be easier and we'd incrementally render stuff, too.

[alecmerdler]

Yep, fixed this by dispatching the loaded event after the first k8sList.

[serenamarie125]

@alecmerdler as we spoke the other day, I wasn't able to identify any other products which have this same situation. Is this something that we can collaborate on together during development from a design perspective?

bug fixed

[serenamarie125]

@alecmerdler looks good so far ... I have a couple of comments
1- is it possible to put a message similar to this above the first row of data ( under the header ) so that the user is informed that the table is still loading?

2- the filter component flashes quite a bit because the numbers are updating, I wonder if there's another alternative to that as well? Does it make sense not to show the filters til the table is completely full ?
3 - do we allow for interaction in the filter/search component in the top right when the table is still loading ?

[ggreer]

Code looks good to me.

[ggreer]

1- is it possible to put a message similar to this above the first row of data ( under the header ) so that the user is informed that the table is still loading?

Yes, but unless it's positioned carefully, it will guarantee that the items will move up when they're done loading.

3 - do we allow for interaction in the filter/search component in the top right when the table is still loading ?

Yes. You can click on filters or start searching and the current results will be filtered. As new data comes in, anything that matches the filters and search will be shown.

[serenamarie125]

I'm ok with this @alecmerdler , and pursuing iterating on the design in a follow up