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

Limit jobs and add pagination #54

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

Conversation

marcosvm
Copy link
Contributor

@marcosvm marcosvm commented Oct 20, 2018

Limits jobs and adds pagination for #49

  • add pagination for jobs in the data layer, with default of 10 jobs
  • add limit of 50 jobs
  • add pagination in the GraphQL layer

Supports basic pagination for jobs adding the query arguments page and size, with queries like:

  jobs(page:3, size: 2)  {    
  
    id    
    repoSlug
    log
  }
}

The data layer will limit the number of jobs to 50 and the default page size is 10.

@coveralls
Copy link

coveralls commented Oct 20, 2018

Coverage Status

Coverage increased (+0.2%) to 83.412% when pulling cc56d9e on marcosvm:marcosvm/limit_jobs into b601645 on elixir-bench:master.

@AndrewDryga
Copy link
Member

AndrewDryga commented Oct 22, 2018

Cursor based pagination is probably not the best option if we allow going deep into the list. Project is public and google would nail our database by indexing last pages: https://use-the-index-luke.com/no-offset. @tallysmartins what do you think?

@tallysmartins
Copy link
Member

tallysmartins commented Oct 25, 2018

Hi, thanks for your feedback @AndrewDryga

I didn't think about this problems regarding cursor based pagination and I didn't get how it avoids google crawling, for both solutions can't we just set up the robots.txt to limit the page indexing?

And the offset-less approach seems to be a better implementation indeed. @marcosvm can you take a look at the article given in the comment from @AndrewDryga ?

@marcosvm
Copy link
Contributor Author

@tallysmartins Sure thing. I'm looking it at it.

@AndrewDryga
Copy link
Member

AndrewDryga commented Oct 25, 2018

There is a package that uses id + timestamp cursors for paging: https://github.com/duffelhq/paginator.

@tallysmartins cursor pagination should not solve issues with crawling, but a core issue why they could nail our DB. Crawlers would index all pages over and over again, following very deep links where people won't ever look (eg. indexing results from 10000 builds ago). If we decide use offset based paging queries to respond to crawlers can be very expensive slowing down an entire system.

This is not very important while we are super small though.

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

4 participants