enhance(api/build): adding the ability to search builds within time constraints #586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #586 +/- ##
==========================================
- Coverage 54.38% 54.32% -0.07%
==========================================
Files 181 181
Lines 15152 15177 +25
==========================================
+ Hits 8241 8245 +4
- Misses 6594 6615 +21
Partials 317 317
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Saw comment about offline conversation, however, would it make more sense to just expose the created, started, finished, etc in query params, then the user can just pick the ranges for the specific timestamp they are interested in.
|
|
I'm not sure what the offline conversation was either since it was a year ago, but I chose |
|
Good point, yeah, might be better to go with this design and expand when needed then. Unless @wass3r remembers the other context, since he mentioned the offline conversation. |
|
For additional context, the pagination was talked about in the offline conversation, so nothing in regards to the time range selection - at least that i can recall. I'm personally ok with the |
| // capture before query parameter if present, default to now | ||
| // | ||
| // nolint: gomnd, lll // ignore magic number and long line length | ||
| before, err := strconv.ParseInt(c.DefaultQuery("before", strconv.FormatInt(time.Now().UTC().Unix(), 10)), 10, 64) |
There was a problem hiding this comment.
question - what are the last two arguments?
There was a problem hiding this comment.
Thats the base (10) and the size in bits (64):
database/postgres/build_list.go
Outdated
| err = c.Postgres. | ||
| Table(constants.TableBuild). | ||
| Where("repo_id = ?", r.GetID()). | ||
| Where("started < ?", before). |
There was a problem hiding this comment.
Something to check: Can you look at this query out in production to see how it behaves from a performance and cost perspective? It's probably ok assuming there's an index on repo_id which would narrow the rows down a lot, but since there's not an index on started, I'm curious how fast those queries and what their cost is.
I'm assuming you know how to grab the generated query.? And then you could throw that in a query tool and run explain analyze $your_query. That'll show how long it takes and its cost. It's probably fine, especially if people don't use this often, but as the database grows over time, this kind of stuff can add up and need to be considered for database tuning and maintenance.
There was a problem hiding this comment.
Probably a good thing to explore 👍
FWIW:
We help minimize the DB query cost on list queries due to the LIMIT condition which comes from pagination:
server/database/postgres/build_list.go
Line 166 in a003a41
There was a problem hiding this comment.
I agree that performance should be explored and validated. A LIMIT doesn't do much if it's paired up with an ORDER statement since the DB will still have to do a full sort on that ORDER column before it can even consider LIMITing the return rows. If you didn't have the order, then a limit on its own would probably be fine, but we have them both together.
There was a problem hiding this comment.
We indeed have repo_id as an index in the DB. I ran a few test queries as well as the explain analyse tool. It seems like the query is fairly performant. I got results that varied from a couple tenths of a millisecond to two milliseconds.
There was a problem hiding this comment.
It's probably worth calling out the indexes are visible from the application code or at least the default indexes. An admin could add more outside this code.
database/postgres/build_list_test.go
Outdated
| ).AddRow(1, 1, 1, 0, "", "", "", 0, 0, 0, 0, "", nil, "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", 0). | ||
| AddRow(2, 1, 2, 0, "", "", "", 0, 0, 0, 0, "", nil, "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", 0) | ||
| ).AddRow(1, 1, 1, 0, "", "", "", 0, 0, 1, 0, "", nil, "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", 0). | ||
| AddRow(2, 1, 2, 0, "", "", "", 0, 0, 2, 0, "", nil, "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", 0) |
There was a problem hiding this comment.
nothing to do here right now, but a comment...this many params is hard to understand and prone to errors. named arguments would be better, or a type passed that contains the parameters. something to think about for future development.
There was a problem hiding this comment.
Named params isn't quite a thing in Go, you can pass a struct or call a function on a struct to access it's fields but in this particular instance we are handicapped by the library. If you look up the NewRow in godoc you'll see it takes that array of strings to define what the columns are and then AddRow takes an array of a custom type which narrows down it down to primitives.
jbrockopp
left a comment
jbrockopp
left a comment
There was a problem hiding this comment.
The code LGTM but had a question on created vs started.
…go-vela/server into i187/add-timestamp-filter-to-get-builds
Adding
beforeandafterquery parameters which take in unix epoch timestamps and fetches builds within those time constraints (if provided). Closes issue 187.