-
Notifications
You must be signed in to change notification settings - Fork 93
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
Extra unnecessary(?) requests happening whenever Recurly_Pager::rewind()
called.
#212
Comments
This is related to what is reported in #207 |
@nerrad hey so i'm curious what's causing the rewinds? are you reusing an iterator? or is it within PHP's foreach? |
yah I had the rewind() in our code originally in a foreach which I refactored because it was silly. It exposed the change though. Also, there's still potential where the iterator could be referenced multiple times in the same request, in which case there doesn't seem to be need to hit the http when calling |
My intention with the change was that for a multi-page iteration rewinding should re-request since the first page's results would have already been discarded. The unintended change I didn't foresee is on an iterator where there's only single page worth of results which would still be in memory, they'd be discarded and re-requested. I could sort of make the argument that if you're rewinding you'd want fresh data but it's definitely a change. I'm open to restoring the old behavior if there's a strong need for it. |
Well personally I think its a pretty big leap to assume that ANY resource using IF you decide to keep the change as is, that will affect the assumptions we've made about the pager, and we'll likely build our own separate class for caching the objects that are retrieved while paging. Regarding this:
Is that the case? After each page retrieval, are the current objects that have been retrieved in the old page(s) discarded? |
@nerrad both the old and new code only maintained a single page worth of results. for multi-page result sets rewinding would re-request the first page. the change in the new code is that rewinding on the first page will now re-request. my assumption is that the client if you're making multiple passes through a result set, it'd probably be best to just use something like |
hmm.. yeah looks like I misread the code. I see I'll rework what we're doing. Thanks for stepping through things with me. |
@nerrad totally, yeah sorry the pagination stuff is kind of a mess. i was trying to fix another bug and clean up as much as i could without breaking anything. there weren't any tests so it was hard to nail down all the prior behaviors. i'm glad it wasn't a huge blocker for you. |
Introduced in this commit: e5bb1ff#diff-91098c9a075e43eeab9310ea7998d870
This appears to have changed the behaviour of
rewind()
so an http request is done ALWAYS when calling rewind() instead of just when theisset($this->_links['start'])
. This means even if one has iterated through all pages, any rewind() will (possibly unnecessarily) do additional http requests to the server instead of resetting the pointer for cached objects.In our case this was a breaking change because we built our consumer based on the previous behaviour where rewind() ONLY loaded from the api if there had been no initial request yet. So we now have lots of places in our code where we started hitting Recurly API limits because rewind() now fully resets the internal caching of the objects and rebuilds on other pages as well.
I don't know if this change was intended or not. But fairly significant regardless.
The text was updated successfully, but these errors were encountered: