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.

Sign up for GitHub

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

Jump to bottom

adding paging to get repositories from the database #1793

Closed
wants to merge 6 commits into from
Closed

adding paging to get repositories from the database #1793

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ed8515
adding paging to get repositories from the database
thiago Sep 13, 2016
56ef552
Move type Feeds to datastore, remove attribute Feed.Id and apply sort…
thiago Sep 14, 2016
239c364
adding paging to get repositories from the database
thiago Sep 15, 2016
670f37e
removing underscore prefix
thiago Sep 15, 2016
64db811
fix merge
Sep 20, 2016
46ceeb6
git push origin remove_limit_reposMerge branch 'bugfix/paginated_repo…
Sep 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions model/feed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ type Feed struct {
Name string `json:"name" meddler:"repo_name"`
FullName string `json:"full_name" meddler:"repo_full_name"`

Id int `json:"id,omitempty" meddler:"build_id,zeroisnull"`
Copy link

Choose a reason for hiding this comment

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

perhaps we sort on Created instead since we already have the field available?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, i dont know why did it! 😊

Number int `json:"number,omitempty" meddler:"build_number,zeroisnull"`
Event string `json:"event,omitempty" meddler:"build_event,zeroisnull"`
Status string `json:"status,omitempty" meddler:"build_status,zeroisnull"`
Expand All @@ -25,3 +26,17 @@ type Feed struct {
Avatar string `json:"author_avatar,omitempty" meddler:"build_avatar,zeroisnull"`
Email string `json:"author_email,omitempty" meddler:"build_email,zeroisnull"`
}

type Feeds []*Feed

Copy link

Choose a reason for hiding this comment

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

can we keep this type in the store/datastore package since it probably only used internally in that package?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, store/datastore/users.go would be a right file? It only used by him...

func (slice Feeds) Len() int {
return len(slice)
}

func (slice Feeds) Less(i, j int) bool {
return slice[i].Id < slice[j].Id;
}

func (slice Feeds) Swap(i, j int) {
slice[i], slice[j] = slice[j], slice[i]
}
21 changes: 12 additions & 9 deletions store/datastore/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,20 @@ func (db *datastore) GetRepoListOf(listof []*model.RepoLite) ([]*model.Repo, err
var (
repos []*model.Repo
args []interface{}
stmt string
err error
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
}
if len(args) > 0 {
err = meddler.QueryAll(db, &repos, fmt.Sprintf(repoListOfQuery, stmt), args...)
_stmt, _args := toList(listof)

for i, stmt := range _stmt{
args = _args[i]
if len(args) > 0 {
var _repos []*model.Repo
err = meddler.QueryAll(db, &_repos, fmt.Sprintf(repoListOfQuery, stmt), args...)
if err != nil{
break
}
repos = append(repos, _repos...)
}
}
return repos, err
}
Expand Down
47 changes: 47 additions & 0 deletions store/datastore/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/drone/drone/model"
"github.com/franela/goblin"
"fmt"
Copy link

Choose a reason for hiding this comment

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

import grouping and sorting :)

)

func TestRepos(t *testing.T) {
Expand Down Expand Up @@ -138,6 +139,52 @@ func TestRepos(t *testing.T) {
g.Assert(count).Equal(2)
})

g.It("Should Get a Repo List Paginated", func() {
_repos := []*model.RepoLite{
{FullName: "bradrydzewski/drone"},
Copy link

Choose a reason for hiding this comment

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

the newer versions of the Go linter will complain when underscores are used in variable names, so we've tried to start removing these from our code. So perhaps repoLiteList instead?

{FullName: "drone/drone"},
}
_repo := []*model.Repo{
Copy link

Choose a reason for hiding this comment

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

perhaps repoList instead of _repo since it is an array. When I see _repo I think it is a single repo

{
UserID: 1,
Owner: "bradrydzewski",
Name: "drone",
FullName: "bradrydzewski/drone",
},
{
UserID: 2,
Owner: "drone",
Name: "drone",
FullName: "drone/drone",
},
}
s.CreateRepo(_repo[0])
s.CreateRepo(_repo[1])

for i := 0; i < 3000; i++{
// find by 3000 repos
_repos = append(_repos, &model.RepoLite{FullName: "octocat/hello-world"+ fmt.Sprint(i)})
// but create only 2000 repos
if i >= 1999 && i < 2999{
continue
}
repo := &model.Repo{
UserID: 2,
Owner: "octocat",
Name: "hello-world" + fmt.Sprint(i),
FullName: "octocat/hello-world" + fmt.Sprint(i),
}
_repo = append(_repo, repo)
s.CreateRepo(repo)
}
repos, err := s.GetRepoListOf(_repos)
g.Assert(err == nil).IsTrue()
g.Assert(len(repos)).Equal(2002)
g.Assert(repos[0].ID).Equal(_repo[0].ID)
g.Assert(repos[1].ID).Equal(_repo[1].ID)
g.Assert(repos[2001].ID).Equal(_repo[2001].ID)
})

g.It("Should Delete a Repo", func() {
repo := model.Repo{
UserID: 1,
Expand Down
52 changes: 32 additions & 20 deletions store/datastore/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/drone/drone/model"
"github.com/russross/meddler"
"sort"
)
Copy link

Choose a reason for hiding this comment

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

imports should be grouped and sorted based on Go style guide. See https://github.com/golang/go/wiki/CodeReviewComments#imports

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I'm newbie in go.

Copy link

Choose a reason for hiding this comment

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

no worries :)

appreciate you taking the time to fix the issue and learn Go along the way!


func (db *datastore) GetUser(id int64) (*model.User, error) {
Expand All @@ -28,39 +29,49 @@ func (db *datastore) GetUserList() ([]*model.User, error) {
func (db *datastore) GetUserFeed(listof []*model.RepoLite) ([]*model.Feed, error) {
var (
args []interface{}
stmt string
err error

feed = []*model.Feed{}
feed = model.Feeds{}
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
_stmt, _args := toList(listof)

for i, stmt := range _stmt{
args = _args[i]
if len(args) > 0 {
var _feed []*model.Feed
err = meddler.QueryAll(db, &_feed, fmt.Sprintf(userFeedQuery, stmt), args...)
if err != nil{
break
}
feed = append(feed, _feed...)
}
}
if len(args) > 0 {
err = meddler.QueryAll(db, &feed, fmt.Sprintf(userFeedQuery, stmt), args...)
sort.Sort(sort.Reverse(feed))
Copy link

Choose a reason for hiding this comment

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

we can probably return feed if len(feed) <= 50. The majority of drone users don't have > 999 repos so we can optimize for this case

Copy link
Author

Choose a reason for hiding this comment

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

Yep, you're right!

limit := 50
if len(feed) < limit{
limit = len(feed)
}
return feed, err
return feed[:limit], err
}

func (db *datastore) GetUserFeedLatest(listof []*model.RepoLite) ([]*model.Feed, error) {
var (
args []interface{}
stmt string
err error

feed = []*model.Feed{}
)
switch meddler.Default {
case meddler.PostgreSQL:
stmt, args = toListPosgres(listof)
default:
stmt, args = toList(listof)
}
if len(args) > 0 {
err = meddler.QueryAll(db, &feed, fmt.Sprintf(userFeedLatest, stmt), args...)
_stmt, _args := toList(listof)

for i, stmt := range(_stmt){
args = _args[i]
if len(args) > 0 {
var _feed []*model.Feed
err = meddler.QueryAll(db, &_feed, fmt.Sprintf(userFeedLatest, stmt), args...)
if err != nil{
break
}
feed = append(feed, _feed...)
}
}
return feed, err
}
Expand Down Expand Up @@ -114,6 +125,7 @@ SELECT
repo_owner
,repo_name
,repo_full_name
,build_id
,build_number
,build_event
,build_status
Expand Down
60 changes: 30 additions & 30 deletions store/datastore/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/drone/drone/model"
"github.com/russross/meddler"
"math"
)
Copy link

Choose a reason for hiding this comment

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

import grouping / ordering


// rebind is a helper function that changes the sql
Expand All @@ -17,7 +18,7 @@ func rebind(query string) string {

qb := []byte(query)
// Add space enough for 5 params before we have to allocate
rqb := make([]byte, 0, len(qb)+5)
rqb := make([]byte, 0, len(qb) + 5)
j := 1
for _, b := range qb {
switch b {
Expand All @@ -38,34 +39,33 @@ func rebind(query string) string {

// helper function that converts a simple repsitory list
// to a sql IN statment.
func toList(listof []*model.RepoLite) (string, []interface{}) {
var size = len(listof)
if size > 999 {
size = 999
listof = listof[:999]
}
var qs = make([]string, size, size)
var in = make([]interface{}, size, size)
for i, repo := range listof {
qs[i] = "?"
in[i] = repo.FullName
}
return strings.Join(qs, ","), in
}
func toList(listof []*model.RepoLite) ([]string, [][]interface{}) {
var total = len(listof)
Copy link

Choose a reason for hiding this comment

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

can we avoid returning a [][]interface{} and instead do something like:

const limit = 999

total := len(listof)
if total == 0 {
    return feed
}

pages := total / limit
if total % limit != 0 {
  pages++
}

for i:=0;i<pages;i++ {
    stmt, args := toList(listof, i*limit, limit)

    var tmpFeed []*model.Feed
    err = meddler.QueryAll(db, &tmpFeed, 
      fmt.Sprintf(userFeedQuery, stmt), args...)
    if err != nil {
      return nil, err
    }

    feed = append(feed, tmpFeed...)
}

if len(feed) <= 50 {
   return feed, nil
}

// sort

return feed[:50], nil

where start and limit indicate where to slice

func toList(listof []*model.RepoLite, start, limit int) ([]string, []interface{}) {
   if len(listof) != limit {
     listof = listof[start,limit]
   }

it could make sense to slice the listof before invoking toList as well ... I don't have big preference either way

Copy link
Author

Choose a reason for hiding this comment

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

This was my first solution... It did not seem attractive for me because the limit variable and the core logic for pagination was replicated on the repos and users functions.

Copy link

@bradrydzewski bradrydzewski Sep 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It did not seem attractive for me because the limit variable

this limit could be a global constant, for example:

const maxPlaceholder = 999

core logic for pagination was replicated

I think this is OK

Copy link

Choose a reason for hiding this comment

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

core logic for pagination was replicated

note that it could always get split out into a separate helper function:

func calculatePagination(total, limit int) (pages int) {
  pages = total / limit
  if total % limit != 0 {
    pages++
  }
  return
}

var limit = 999
var pagesCount = int(math.Ceil(float64(total) / float64(limit)))
Copy link

Choose a reason for hiding this comment

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

can we avoid the float64 allocation and conversion here?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure, because 10 / 3 = 3 but 10.0 / 3.0 = 3.3333... with ceiling to 4 pages.


// helper function that converts a simple repository list
// to a sql IN statement compatible with postgres.
func toListPosgres(listof []*model.RepoLite) (string, []interface{}) {
var size = len(listof)
if size > 999 {
size = 999
listof = listof[:999]
}
var qs = make([]string, size, size)
var in = make([]interface{}, size, size)
for i, repo := range listof {
qs[i] = "$" + strconv.Itoa(i+1)
in[i] = repo.FullName
var qs = make([]string, pagesCount, pagesCount)
Copy link

Choose a reason for hiding this comment

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

shouldn't we have [][]string to correspond to [][]interface{}? [0][]string may have a different length than [1][]interface{} right? The first page, index [0] could have 999 items while the second page, index [1] could have != 999 items and therefore needs a different placeholder lenght

var in = make([][]interface{}, pagesCount, pagesCount)

for page := 0; page < int(pagesCount); page++ {
var start = page * limit
var end = (page * limit) + limit
if end > total{
end = total
}
var size = end - start
var _qs = make([]string, size, size)
var _in = make([]interface{}, size, size)
for i, repo := range listof[start:end] {
if meddler.Default == meddler.PostgreSQL {
Copy link

Choose a reason for hiding this comment

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

this seems a bit inefficient since it could add thousands of extra evaluations depending on the size of the list. This was the benefit of having this split into two functions before. The prior implementation was more efficient at the expense of being dry (which was on purpose)

_qs[i] = "$" + strconv.Itoa(i + 1)
}else{
_qs[i] = "?"
}
_in[i] = repo.FullName
}
qs = append(qs, strings.Join(_qs, ","))
in = append(in, _in)
}
return strings.Join(qs, ","), in
}
return qs, in
}