I feel support ?offset=<number> should be deleted. We got harvested recently and the fact that offset can be used took a lot of datastore processing power from our instance.
So my take:
1. Add to def search() the option to sort in decreasing timestamp. Let's say it's "order=decreasing" here for simplicity. 2. Fix /search UI to be consistent. 3. Links "Open Issues", "Closed Issues" and "All Issues" should be updated to be redirected to /search?<relevant_query>. 4. def index() should redirect to /search?closed=3&order=decreasing. Determine: Handle old query args or don't bother? 5. def all() should redirect to /search?closed=1&order=decreasing. Determine: Handle old query args or don't bother? 6. def _paginate_issue() can then be deleted. Yay!
On Mon, Jan 30, 2012 at 9:45 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote: > Hi
> I feel support ?offset=<number> should be deleted. We got harvested recently > and the fact that offset can be used took a lot of datastore processing > power from our instance.
> So my take:
> Add to def search() the option to sort in decreasing timestamp. Let's say > it's "order=decreasing" here for simplicity. > Fix /search UI to be consistent. > Links "Open Issues", "Closed Issues" and "All Issues" should be updated to > be redirected to /search?<relevant_query>. > def index() should redirect to /search?closed=3&order=decreasing. Determine: > Handle old query args or don't bother? > def all() should redirect to /search?closed=1&order=decreasing. Determine: > Handle old query args or don't bother? > def _paginate_issue() can then be deleted. Yay!
> Any opinion?
> M-A
> -- > You received this message because you are subscribed to the Google Groups > "codereview-discuss" group. > To post to this group, send email to codereview-discuss@googlegroups.com. > To unsubscribe from this group, send email to > codereview-discuss+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/codereview-discuss?hl=en.
On Mon, Jan 30, 2012 at 8:45 PM, Marc-Antoine Ruel <mar...@chromium.org>wrote:
> Hi
> I feel support ?offset=<number> should be deleted. We got harvested > recently and the fact that offset can be used took a lot of datastore > processing power from our instance.
Is there any way to guard AppEngine apps from such attacks?
So my take:
> 1. Add to def search() the option to sort in decreasing timestamp. > Let's say it's "order=decreasing" here for simplicity. > 2. Fix /search UI to be consistent. > 3. Links "Open Issues", "Closed Issues" and "All Issues" should be > updated to be redirected to /search?<relevant_query>. > 4. def index() should redirect to /search?closed=3&order=decreasing. > Determine: Handle old query args or don't bother? > 5. def all() should redirect to > /search?closed=1&order=decreasing. Determine: Handle old query args or > don't bother? > 6. def _paginate_issue() can then be deleted. Yay!
In general making all() and index() to be redirects to unified and paginated search output seems ok, but it will also double amount of requests unless all menu links are replaced with /search URLs. -- anatoly t.
Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
> I feel dense -- I do not see how you would resolve viewing the rest of > the issues if the first page doesn't hold them all.
> FWIW if you want to kill offset shouldn't you also kill limit? It can > also be abused.
limit can't be abused in the sense that it's limited to 1000. For some odd reason, it seems that offset > 1000 isn't refused right away and instead caused the instance to spin for 60 seconds, on every request. The guy was eventually using offset over 100000. The online log dashboard isn't useful to know what happened earlier, I can only see last few hours.
The main difference between _paginate_issue() and _paginate_issues_with_cursor() is that a cursor is much more efficient and coherent than an offset.
Le 30 janvier 2012 13:26, anatoly techtonik <techto...@gmail.com> a écrit :
> Is there any way to guard AppEngine apps from such attacks?
Yes, dos.yaml. I used it but it's manual.
There could be a middleware to throttle some kinds of heavy weight requests from a single IP (let's ignore ddos here) but even that is lower priority to me.
Le 30 janvier 2012 13:26, anatoly techtonik <techto...@gmail.com> a écrit :
> In general making all() and index() to be redirects to unified and > paginated search output seems ok, but it will also double amount of > requests unless all menu links are replaced with /search URLs.
Replacing the links was specified as step 3. The rest is priorization.
> On Mon, Jan 30, 2012 at 9:45 AM, Marc-Antoine Ruel <mar...@chromium.org> > wrote: > > Hi
> > I feel support ?offset=<number> should be deleted. We got harvested > recently > > and the fact that offset can be used took a lot of datastore processing > > power from our instance.
> > So my take:
> > Add to def search() the option to sort in decreasing timestamp. Let's say > > it's "order=decreasing" here for simplicity. > > Fix /search UI to be consistent. > > Links "Open Issues", "Closed Issues" and "All Issues" should be updated > to > > be redirected to /search?<relevant_query>. > > def index() should redirect to /search?closed=3&order=decreasing. > Determine: > > Handle old query args or don't bother? > > def all() should redirect to > /search?closed=1&order=decreasing. Determine: > > Handle old query args or don't bother? > > def _paginate_issue() can then be deleted. Yay!
> > Any opinion?
> > M-A
> > -- > > You received this message because you are subscribed to the Google Groups > > "codereview-discuss" group. > > To post to this group, send email to codereview-discuss@googlegroups.com > . > > To unsubscribe from this group, send email to > > codereview-discuss+unsubscribe@googlegroups.com. > > For more options, visit this group at > > http://groups.google.com/group/codereview-discuss?hl=en.
On Mon, Jan 30, 2012 at 11:18 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote: > Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
>> I feel dense -- I do not see how you would resolve viewing the rest of >> the issues if the first page doesn't hold them all.
>> FWIW if you want to kill offset shouldn't you also kill limit? It can >> also be abused.
> limit can't be abused in the sense that it's limited to 1000. For some odd > reason, it seems that offset > 1000 isn't refused right away and instead > caused the instance to spin for 60 seconds, on every request. The guy was > eventually using offset over 100000. The online log dashboard isn't useful > to know what happened earlier, I can only see last few hours.
I guess he was just following all links in the UI?
We could replace the next/prev links with links based on cursors, then there would be no cost to skip to the desired point in the query.
> The main difference between _paginate_issue() and > _paginate_issues_with_cursor() is that a cursor is much more efficient and > coherent than an offset.
Duh, so we already use cursors? Forget what I said then. :-)
> On Mon, Jan 30, 2012 at 11:18 AM, Marc-Antoine Ruel <mar...@chromium.org> > wrote: > > Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
> >> I feel dense -- I do not see how you would resolve viewing the rest of > >> the issues if the first page doesn't hold them all.
> >> FWIW if you want to kill offset shouldn't you also kill limit? It can > >> also be abused.
> > limit can't be abused in the sense that it's limited to 1000. For some > odd > > reason, it seems that offset > 1000 isn't refused right away and instead > > caused the instance to spin for 60 seconds, on every request. The guy was > > eventually using offset over 100000. The online log dashboard isn't > useful > > to know what happened earlier, I can only see last few hours.
> I guess he was just following all links in the UI?
> We could replace the next/prev links with links based on cursors, then > there would be no cost to skip to the desired point in the query.
> > The main difference between _paginate_issue() and > > _paginate_issues_with_cursor() is that a cursor is much more efficient > and > > coherent than an offset.
> Duh, so we already use cursors? Forget what I said then. :-)
/all uses offsets, /search uses cursors. This results in mostly similar code. Hence my original proposition.
On Mon, Jan 30, 2012 at 11:32 AM, Marc-Antoine Ruel <mar...@chromium.org> wrote: > Le 30 janvier 2012 14:25, Guido van Rossum <gu...@python.org> a écrit :
>> On Mon, Jan 30, 2012 at 11:18 AM, Marc-Antoine Ruel <mar...@chromium.org> >> wrote: >> > Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
>> >> I feel dense -- I do not see how you would resolve viewing the rest of >> >> the issues if the first page doesn't hold them all.
>> >> FWIW if you want to kill offset shouldn't you also kill limit? It can >> >> also be abused.
>> > limit can't be abused in the sense that it's limited to 1000. For some >> > odd >> > reason, it seems that offset > 1000 isn't refused right away and instead >> > caused the instance to spin for 60 seconds, on every request. The guy >> > was >> > eventually using offset over 100000. The online log dashboard isn't >> > useful >> > to know what happened earlier, I can only see last few hours.
>> I guess he was just following all links in the UI?
>> We could replace the next/prev links with links based on cursors, then >> there would be no cost to skip to the desired point in the query.
>> > The main difference between _paginate_issue() and >> > _paginate_issues_with_cursor() is that a cursor is much more efficient >> > and >> > coherent than an offset.
>> Duh, so we already use cursors? Forget what I said then. :-)
> /all uses offsets, /search uses cursors. This results in mostly similar > code. Hence my original proposition.
> Le 30 janvier 2012 13:12, Guido van Rossum <gu...@python.org> a écrit :
> I feel dense -- I do not see how you would resolve viewing the rest of >> the issues if the first page doesn't hold them all.
>> FWIW if you want to kill offset shouldn't you also kill limit? It can >> also be abused.
> limit can't be abused in the sense that it's limited to 1000. For some odd > reason, it seems that offset > 1000 isn't refused right away and instead > caused the instance to spin for 60 seconds, on every request. The guy was > eventually using offset over 100000. The online log dashboard isn't useful > to know what happened earlier, I can only see last few hours.
> The main difference between _paginate_issue() and > _paginate_issues_with_cursor() is that a cursor is much more efficient and > coherent than an offset.
You didn't mention cursors in the first place, so that's ok for me. =) If I understand correctly, cursor just doesn't allow to go to 15th page directly, is that right?
> Le 30 janvier 2012 13:26, anatoly techtonik <techto...@gmail.com> a écrit > :
> Is there any way to guard AppEngine apps from such attacks?
> Yes, dos.yaml. I used it but it's manual.
> There could be a middleware to throttle some kinds of heavy weight > requests from a single IP (let's ignore ddos here) but even that is lower > priority to me.
I wonder if such middleware should be provided by AppEngine to protect customers from attacks that directly empty their bank accounts?
Le 31 janvier 2012 03:13, anatoly techtonik <techto...@gmail.com> a écrit :
> On Mon, Jan 30, 2012 at 10:18 PM, Marc-Antoine Ruel <mar...@chromium.org>wrote:
>> The main difference between _paginate_issue() and >> _paginate_issues_with_cursor() is that a cursor is much more efficient and >> coherent than an offset.
> You didn't mention cursors in the first place, so that's ok for me. =) If > I understand correctly, cursor just doesn't allow to go to 15th page > directly, is that right?
Exactly, you have to follow the pages one at a time and can't skip to a random offset. It's a minor annoyance for greater good.
Sorry I somehow took cursor usage as "common knowledge". :)
Anyway, I was just touching water, I'll take a look how messy it is to complete.
> Le 30 janvier 2012 13:26, anatoly techtonik <techto...@gmail.com> a écrit >> :
>> Is there any way to guard AppEngine apps from such attacks?
>> Yes, dos.yaml. I used it but it's manual.
>> There could be a middleware to throttle some kinds of heavy weight >> requests from a single IP (let's ignore ddos here) but even that is lower >> priority to me.
> I wonder if such middleware should be provided by AppEngine to protect > customers from attacks that directly empty their bank accounts?
I don't think it's AE's duty to do that. But I wouldn't be surprised if someone had already written something like that for django. Something that piggies back on appstats and detect 10 minutes average sudden increase or send IM on sudden error increases.
