Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196453002:
Milo: Console view prototype (Closed)
|
Created:
4 years, 4 months ago by Ryan Tseng Modified:
4 years, 4 months ago CC:
andrew.wang, chromium-reviews, infra-reviews+luci-go_chromium.org, M-A Ruel, tandrii+luci-go_chromium.org, todd Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-go@master Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionMilo: Console view prototype
This is pretty hacked up. Namely:
* This does a 25 build query for _each_ builder, and then filter out the correct revision
* This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process)
* Hits gitiles for every query (70ms)
* Only buildbot is supported
* Config is hard-coded instead of being loaded from luci-cfg
All of the issues above will be addressed later, this is mostly to get a demo out.
BUG=632516
Committed: https://github.com/luci/luci-go/commit/19b5ff60eca7ed2c7a3f2b71cd1b6ab99d3e04b7
Patch Set 1 #Patch Set 2 : Move git to it's own package #Patch Set 3 : Fixes #Patch Set 4 : rebase #Patch Set 5 : Rebase fix #
Total comments: 64
Patch Set 6 : Review, cleanup #Patch Set 7 : fixes #
Total comments: 39
Patch Set 8 : Reviews #Messages
Total messages: 56 (39 generated)
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Milo: Console view prototype BUG=632516 ========== to ========== Milo: Console view prototype This is pretty hacked up. Namely: * This does a 25 build query for _each_ builder, and then filter out the correct revision * This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process) * Hits gerrit for every query (70ms) * Only buildbot is supported * Config is hard-coded instead of being loaded from luci-cfg All of the issues above will be addressed later, this is mostly to get a demo out. BUG=632516 ==========
hinoka@google.com changed reviewers: + estaab@chromium.org, nodir@chromium.org
Fixes
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Demo here: https://982-d7d2bb3-tainted-hinoka-dot-luci-milo.appspot.com/console/asdf ptal! There's a lot more work to be done so this is definitely not ready for anyone's prying eyes, but it's a starting point.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Meow
rebase
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-go Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/306a9a4234928510)
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hinoka@chromium.org changed reviewers: + hinoka@chromium.org
Ping?
https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... File appengine/cmd/milo/buildbot/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:21: func getFullBuilds(c context.Context, masterName, builderName string, finished bool) ([]*buildbotBuild, error) { why "Full"? https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:43: for _, builder := range builders { this loop may take a lot of time because of master unzipping and json parsing. consider timing it https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:44: f := strings.SplitN(builder.Name, "/", 2) validate that len(f) == 2, return error if not so https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:58: return nil, fmt.Errorf("Internal masters not supported") if someone defines a config that references an internal master and then milo starts breaking with "Internal masters not supported", this way an adversary can sniff internal master names. Just saying, not sure what you can with it with the current design https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:74: builderName := f[1] this will panic if builder.Name does not have a slash https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:77: builds, err := getFullBuilds(c, master, builderName, true) it won't show running builds? also does not show a build for a commit if it was more than 25 builds in the past https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:85: if build.Sourcestamp.Revision == commit { this is O(n^2) make a map commit->j before fanOutIn and then use it inside `for _, build := range builds` loop https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:86: results[j][i] = &resp.ConsoleBuild{ this code is racy: it mutates results[j] in different goroutines https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:98: results[j][i] = &resp.ConsoleBuild{Status: *currentStatus} I don't understand this logic. It might be simpler and correct to do the following: after fanOutIn, for each builder, find first non-nil status among all commits https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:110: remove blank line https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... File appengine/cmd/milo/console/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:43: panic(fmt.Errorf("Unrecognized module %s", module)) a panic kinda implies that the caller really knows what module they want. If so, I am not sure what's the value of this function if the caller can just call buildbot.GetConsoleBuilds or buildbucket.GetConsoleBuilds https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:61: *resp.Console, error) { i think it would fit prev line https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:68: t2 := clock.Now(c) please give more meaning full names to t variables, e.g. tStarted, tCommitsLoaded you won't have to rename t variables when you insert a new t in between existing t's https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:69: log.Debugf(c, fmt.Sprintf("Gerrit took %f seconds.", t2.Sub(t1).Seconds())) s/Gerrit/Loading commits/ https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:69: log.Debugf(c, fmt.Sprintf("Gerrit took %f seconds.", t2.Sub(t1).Seconds())) log.Debugf accepts a format string, so no need for Sprintf https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:69: log.Debugf(c, fmt.Sprintf("Gerrit took %f seconds.", t2.Sub(t1).Seconds())) use native representation of durations: took %s", r2.Sub(t1)) https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:76: cb, err := GetConsoleBuilds(c, "buildbot", def.Builders, commitNames) time it? https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... File appengine/cmd/milo/console/html.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:38: Repository: "chromium.googlesource.com/chromium/src", please make it a valid URL, prepend https:// https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:64: // Render Renders the console page. s/Renders/renders/ https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:75: t3 := clock.Now(c) rename to start https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:81: log.Debugf(c, "Console took %f seconds.", t4.Sub(t3).Seconds()) use clock.Since(c, start) https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:81: log.Debugf(c, "Console took %f seconds.", t4.Sub(t3).Seconds()) use native representation of durations https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... File appengine/cmd/milo/git/gerrit.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:22: type GerritCommit struct { this is Gitiles, not Gerrit https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:27: Author struct { consider defining a type so you don't copy-paste type definitions inline an alternative is to not parse Committer because you don't use it https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:42: func GetCommits(c context.Context, repo, treeish string) ([]resp.Commit, error) { repo looks like an URL (chromium.googlesource.com/chromium/src), but it is not because does not have scheme. Consider renaming to repoURL and verify that it starts with https:// https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:43: URL := fmt.Sprintf("https://%s/+log/%s?format=JSON", repo, treeish) add /a/ in the beginning, so you get a bigger quota also make sure to whitelist your app in gitiles internal config (ask me how) https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:44: client := transport.GetClient(c) this needs to be rebased https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:53: trash := make([]byte, 4) at least add a comment what is the prefix https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:55: d := json.NewDecoder(r.Body) inline d https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:56: cms := GerritCommit{} what cms stands for? https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:68: Repo: URL, URL is not a repo URL I think you meant repoURL https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/resp... File appengine/cmd/milo/resp/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/resp... appengine/cmd/milo/resp/console.go:6: comments in this file would be great
Review, cleanup
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fixes
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
See https://1031-50a0896-tainted-hinoka-dot-luci-milo.appspot.com/console/asdf for PS7 https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... File appengine/cmd/milo/buildbot/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:21: func getFullBuilds(c context.Context, masterName, builderName string, finished bool) ([]*buildbotBuild, error) { On 2016/08/03 20:26:16, nodir wrote: > why "Full"? Not sure, I think it was arbitrary. I guess full vs. summary. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:43: for _, builder := range builders { On 2016/08/03 20:26:17, nodir wrote: > this loop may take a lot of time because of master unzipping and json parsing. > consider timing it Actually this entire loop isn't used... I forgot why this was here. I think it was to resolve wildcards like chromium.linux/* builders, a feature that I later stripped out. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:44: f := strings.SplitN(builder.Name, "/", 2) On 2016/08/03 20:26:17, nodir wrote: > validate that len(f) == 2, return error if not so Deleted https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:58: return nil, fmt.Errorf("Internal masters not supported") On 2016/08/03 20:26:17, nodir wrote: > if someone defines a config that references an internal master and then milo > starts breaking with "Internal masters not supported", this way an adversary can > sniff internal master names. Just saying, not sure what you can with it with the > current design Acknowledged. Probably should just treat the builder as "not found". https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:74: builderName := f[1] On 2016/08/03 20:26:16, nodir wrote: > this will panic if builder.Name does not have a slash Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:77: builds, err := getFullBuilds(c, master, builderName, true) On 2016/08/03 20:26:16, nodir wrote: > it won't show running builds? also does not show a build for a commit if it was > more than 25 builds in the past Yes that is true, but it hasn't been a problem for the demo. Added comment to indicate this is meant to be a hack. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:85: if build.Sourcestamp.Revision == commit { On 2016/08/03 20:26:17, nodir wrote: > this is O(n^2) > make a map commit->j before fanOutIn and then use it inside `for _, build := > range builds` loop Logic for filling in-between builds on L97 would break then. I could make a map of commit -> build index to make this O(N), but in terms of performance speed, L77 takes 95% of the time in this goroutine, so it probably doesn't matter too much to fix this. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:86: results[j][i] = &resp.ConsoleBuild{ On 2016/08/03 20:26:17, nodir wrote: > this code is racy: it mutates results[j] in different goroutines Each goroutine fully owns a space in [i], so I don't think they would ever conflict? Also the entire matrix is pre-allocated in L64-67. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:98: results[j][i] = &resp.ConsoleBuild{Status: *currentStatus} On 2016/08/03 20:26:17, nodir wrote: > I don't understand this logic. It might be simpler and correct to do the > following: after fanOutIn, for each builder, find first non-nil status among all > commits Each goroutine processes a single builder. This just remembers the result of the commit prior, and fills in the status if there isn't a matching build, so that the entire matrix is filled out. This isn't used for the "overall" status of the builder. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:110: On 2016/08/03 20:26:17, nodir wrote: > remove blank line Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... File appengine/cmd/milo/console/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:43: panic(fmt.Errorf("Unrecognized module %s", module)) On 2016/08/03 20:26:17, nodir wrote: > a panic kinda implies that the caller really knows what module they want. If so, > I am not sure what's the value of this function if the caller can just call > buildbot.GetConsoleBuilds or buildbucket.GetConsoleBuilds The idea of this function is to map strings to linked modules. The caller knows the name of the module, but unlike python it cannot resolve the name of a module to an actual go package. This function is supposed to serve as the map. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:61: *resp.Console, error) { On 2016/08/03 20:26:17, nodir wrote: > i think it would fit prev line Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:68: t2 := clock.Now(c) On 2016/08/03 20:26:17, nodir wrote: > please give more meaning full names to t variables, e.g. tStarted, > tCommitsLoaded > you won't have to rename t variables when you insert a new t in between existing > t's Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:69: log.Debugf(c, fmt.Sprintf("Gerrit took %f seconds.", t2.Sub(t1).Seconds())) On 2016/08/03 20:26:17, nodir wrote: > log.Debugf accepts a format string, so no need for Sprintf Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/console.go:76: cb, err := GetConsoleBuilds(c, "buildbot", def.Builders, commitNames) On 2016/08/03 20:26:17, nodir wrote: > time it? Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... File appengine/cmd/milo/console/html.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:38: Repository: "chromium.googlesource.com/chromium/src", On 2016/08/03 20:26:17, nodir wrote: > please make it a valid URL, prepend https:// Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:64: // Render Renders the console page. On 2016/08/03 20:26:17, nodir wrote: > s/Renders/renders/ Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:75: t3 := clock.Now(c) On 2016/08/03 20:26:17, nodir wrote: > rename to start Removing this, the impl has timing logs https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/cons... appengine/cmd/milo/console/html.go:81: log.Debugf(c, "Console took %f seconds.", t4.Sub(t3).Seconds()) On 2016/08/03 20:26:17, nodir wrote: > use clock.Since(c, start) Removed https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... File appengine/cmd/milo/git/gerrit.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:22: type GerritCommit struct { On 2016/08/03 20:26:18, nodir wrote: > this is Gitiles, not Gerrit Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:27: Author struct { On 2016/08/03 20:26:18, nodir wrote: > consider defining a type so you don't copy-paste type definitions inline > > an alternative is to not parse Committer because you don't use it Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:42: func GetCommits(c context.Context, repo, treeish string) ([]resp.Commit, error) { On 2016/08/03 20:26:18, nodir wrote: > repo looks like an URL (chromium.googlesource.com/chromium/src), but it is not > because does not have scheme. Consider renaming to repoURL and verify that it > starts with https:// Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:43: URL := fmt.Sprintf("https://%s/+log/%s?format=JSON", repo, treeish) On 2016/08/03 20:26:18, nodir wrote: > add /a/ in the beginning, so you get a bigger quota > also make sure to whitelist your app in gitiles internal config (ask me how) Done. App is already whitelisted. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:44: client := transport.GetClient(c) On 2016/08/03 20:26:18, nodir wrote: > this needs to be rebased ...? https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:53: trash := make([]byte, 4) On 2016/08/03 20:26:18, nodir wrote: > at least add a comment what is the prefix Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:55: d := json.NewDecoder(r.Body) On 2016/08/03 20:26:18, nodir wrote: > inline d Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:56: cms := GerritCommit{} On 2016/08/03 20:26:18, nodir wrote: > what cms stands for? commits https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/git/... appengine/cmd/milo/git/gerrit.go:68: Repo: URL, On 2016/08/03 20:26:18, nodir wrote: > URL is not a repo URL > I think you meant repoURL Done. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/resp... File appengine/cmd/milo/resp/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/resp... appengine/cmd/milo/resp/console.go:6: On 2016/08/03 20:26:18, nodir wrote: > comments in this file would be great Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping? This blocks milo/luci refactor :)
Description was changed from ========== Milo: Console view prototype This is pretty hacked up. Namely: * This does a 25 build query for _each_ builder, and then filter out the correct revision * This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process) * Hits gerrit for every query (70ms) * Only buildbot is supported * Config is hard-coded instead of being loaded from luci-cfg All of the issues above will be addressed later, this is mostly to get a demo out. BUG=632516 ========== to ========== Milo: Console view prototype This is pretty hacked up. Namely: * This does a 25 build query for _each_ builder, and then filter out the correct revision * This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process) * Hits gitiles for every query (70ms) * Only buildbot is supported * Config is hard-coded instead of being loaded from luci-cfg All of the issues above will be addressed later, this is mostly to get a demo out. BUG=632516 ==========
lgtm to unblock repo reorg and because this is a prototype, but this implementation has several design problems. we will need to revise it https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... File appengine/cmd/milo/buildbot/console.go (right): https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:77: builds, err := getFullBuilds(c, master, builderName, true) On 2016/08/03 21:55:39, hinoka wrote: > On 2016/08/03 20:26:16, nodir wrote: > > it won't show running builds? also does not show a build for a commit if it > was > > more than 25 builds in the past > > Yes that is true, but it hasn't been a problem for the demo. Added comment to > indicate this is meant to be a hack. ok, makes sense now https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:86: results[j][i] = &resp.ConsoleBuild{ On 2016/08/03 21:55:39, hinoka wrote: > On 2016/08/03 20:26:17, nodir wrote: > > this code is racy: it mutates results[j] in different goroutines > > Each goroutine fully owns a space in [i], so I don't think they would ever > conflict? Also the entire matrix is pre-allocated in L64-67. Acknowledged. https://codereview.chromium.org/2196453002/diff/80001/appengine/cmd/milo/buil... appengine/cmd/milo/buildbot/console.go:98: results[j][i] = &resp.ConsoleBuild{Status: *currentStatus} On 2016/08/03 21:55:39, hinoka wrote: > On 2016/08/03 20:26:17, nodir wrote: > > I don't understand this logic. It might be simpler and correct to do the > > following: after fanOutIn, for each builder, find first non-nil status among > all > > commits > > Each goroutine processes a single builder. This just remembers the result of > the commit prior, and fills in the status if there isn't a matching build, so > that the entire matrix is filled out. This isn't used for the "overall" status > of the builder. this does not sound correct. if there was no build for a commit, we cannot tell if it was greed or red, so we should not lie about it to the user by assuming that the build result would be the same as build result for the next commit. If we properly handle merged builds, however, then we won't have to do this filling https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... File appengine/cmd/milo/buildbot/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:37: [][]*resp.ConsoleBuild, error) { document what indexes in the return value mean https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:49: f := strings.SplitN(builder.Name, "/", 2) i am not sure what f stands for https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:52: return fmt.Errorf("%s is an invalid builder name", builder.Name) builder var is captured, so there is a race you need to do `builder := builder` in the beginning of the loop https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:68: if build.Sourcestamp.Revision == commit { why does this have only one source stamp? a build may span multiple commits if buildbot build requests for different commits got merged into one build https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:69: results[j][i] = &resp.ConsoleBuild{ since there is no "break" in the end of this `if`, results[i][j] can be overwritten by another build in |builds| that has same revision this also demonstrates a design problem: what if there are multiple builds for the same commit? do we show an arbitrary one? (that's not good) maybe the return value should be [][][]resp.ConsoleBuild https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... File appengine/cmd/milo/console/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:54: tGerrit := clock.Now(c) it is gitiles, not gerrit https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... File appengine/cmd/milo/resp/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... appengine/cmd/milo/resp/console.go:36: Category string // TODO(hinoka): This should be a list? yes, it should be a slice here, but a pipe-delimited string in the luci-config https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... appengine/cmd/milo/resp/console.go:44: Build []*ConsoleBuild consider making it [][]*ConsoleBuild since there may be multiple builds for the same commits. If there is >1 builds in a cell, it should be handled by the template, not other parts of the code
I just saw nodir's reply so I'm just sending my comments as-is. He went more in-depth so treat his as higher priority if they conflict. :) https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... File appengine/cmd/milo/console/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:19: func resolveBuilders(c context.Context, builderSpec []string) []resp.BuilderRef { what's the format of builderSpec? Should this be a struct instead? https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:40: //case "buildbucket": remove commented code https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:47: func console(c context.Context, def *ConsoleDef) (*resp.Console, error) { How much of this do you want to keep? I want to know where I should do a detailed review vs quick scan. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... File appengine/cmd/milo/console/html.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/html.go:22: func (x Console) GetTemplateName(t settings.Theme) string { Why is this a method and not just a function? https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/html.go:34: var def = ConsoleDef{ s/def/chromiumConsoleDef/ so it's clear and so it's easier to recognize that we need to delete it later. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/fro... File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/fro... appengine/cmd/milo/frontend/static/buildbot/css/default.css:461: .exception, .retry, tr:nth-child(even) td.Status-Exception { lowercase status for consistency? https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... File appengine/cmd/milo/git/gitiles.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:18: type Repo struct { docs. doesn't lint warn about this? https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:62: u.Path = "a/" + u.Path u isn't used after this line? https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:66: func GetCommits(c context.Context, repoURL, treeish string) ([]resp.Commit, error) { docs https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:87: // TODO(hinoka): Follow paging add more details https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:99: // TODO(hinoka): The other stuff add more details https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... File appengine/cmd/milo/resp/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... appengine/cmd/milo/resp/console.go:36: Category string // TODO(hinoka): This should be a list? Does it need to be a list to support nested categories?
Reviews
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... File appengine/cmd/milo/buildbot/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:37: [][]*resp.ConsoleBuild, error) { On 2016/08/04 23:13:12, nodir wrote: > document what indexes in the return value mean Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:49: f := strings.SplitN(builder.Name, "/", 2) On 2016/08/04 23:13:13, nodir wrote: > i am not sure what f stands for Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:52: return fmt.Errorf("%s is an invalid builder name", builder.Name) On 2016/08/04 23:13:12, nodir wrote: > builder var is captured, so there is a race > you need to do `builder := builder` in the beginning of the loop Oh good catch https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:68: if build.Sourcestamp.Revision == commit { On 2016/08/04 23:13:12, nodir wrote: > why does this have only one source stamp? a build may span multiple commits if > buildbot build requests for different commits got merged into one build This is buildbot's own concept of sourcestamp, that is, the revision under testing. This is different than the concept of build request merging. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/bui... appengine/cmd/milo/buildbot/console.go:69: results[j][i] = &resp.ConsoleBuild{ On 2016/08/04 23:13:12, nodir wrote: > since there is no "break" in the end of this `if`, results[i][j] can be > overwritten by another build in |builds| that has same revision > > this also demonstrates a design problem: what if there are multiple builds for > the same commit? do we show an arbitrary one? (that's not good) > > maybe the return value should be [][][]resp.ConsoleBuild I think we should pick the latest one (This code won't pick the latest one, but that's okay for now). But I agree we should discuss what should happen with multiple builds, particularly since DM has native support for multiple attempts on the same quest. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... File appengine/cmd/milo/console/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:19: func resolveBuilders(c context.Context, builderSpec []string) []resp.BuilderRef { On 2016/08/04 23:21:24, estaab wrote: > what's the format of builderSpec? Should this be a struct instead? Oops dead code... https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:40: //case "buildbucket": On 2016/08/04 23:21:24, estaab wrote: > remove commented code Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:47: func console(c context.Context, def *ConsoleDef) (*resp.Console, error) { On 2016/08/04 23:21:24, estaab wrote: > How much of this do you want to keep? I want to know where I should do a > detailed review vs quick scan. Parts I'm keeping: * Interface between console/console.go -> buildbot/console.go * Interface between console/console.go -> git/gitiles.go * resp/console.go * settings/func.go: code that resolves list of builders into a "tree" Parts that are disposable: * buildbot/console.go implementation https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/console.go:54: tGerrit := clock.Now(c) On 2016/08/04 23:13:13, nodir wrote: > it is gitiles, not gerrit Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... File appengine/cmd/milo/console/html.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/html.go:22: func (x Console) GetTemplateName(t settings.Theme) string { On 2016/08/04 23:21:25, estaab wrote: > Why is this a method and not just a function? Milo design: A themedHandler is a "class" that implements two functions, GetTemplateName() and Render(). At the top level milo.go only references the classes that are implemented. dnj suggested this pattern and I bought into it. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/con... appengine/cmd/milo/console/html.go:34: var def = ConsoleDef{ On 2016/08/04 23:21:24, estaab wrote: > s/def/chromiumConsoleDef/ so it's clear and so it's easier to recognize that we > need to delete it later. Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/fro... File appengine/cmd/milo/frontend/static/buildbot/css/default.css (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/fro... appengine/cmd/milo/frontend/static/buildbot/css/default.css:461: .exception, .retry, tr:nth-child(even) td.Status-Exception { On 2016/08/04 23:21:25, estaab wrote: > lowercase status for consistency? Oh this was a bug, no wonder it wasn't working lol. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... File appengine/cmd/milo/git/gitiles.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:18: type Repo struct { On 2016/08/04 23:21:25, estaab wrote: > docs. doesn't lint warn about this? They're types not methods, so apparently not. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:62: u.Path = "a/" + u.Path On 2016/08/04 23:21:25, estaab wrote: > u isn't used after this line? Good catch. Fixed. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:66: func GetCommits(c context.Context, repoURL, treeish string) ([]resp.Commit, error) { On 2016/08/04 23:21:25, estaab wrote: > docs Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:87: // TODO(hinoka): Follow paging On 2016/08/04 23:21:25, estaab wrote: > add more details Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/git... appengine/cmd/milo/git/gitiles.go:99: // TODO(hinoka): The other stuff On 2016/08/04 23:21:25, estaab wrote: > add more details Done. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... File appengine/cmd/milo/resp/console.go (right): https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... appengine/cmd/milo/resp/console.go:36: Category string // TODO(hinoka): This should be a list? On 2016/08/04 23:21:25, estaab wrote: > Does it need to be a list to support nested categories? It doesn't technically need to, the callee can split pipe deliminated strings. https://codereview.chromium.org/2196453002/diff/120001/appengine/cmd/milo/res... appengine/cmd/milo/resp/console.go:44: Build []*ConsoleBuild On 2016/08/04 23:13:13, nodir wrote: > consider making it [][]*ConsoleBuild since there may be multiple builds for the > same commits. If there is >1 builds in a cell, it should be handled by the > template, not other parts of the code Let's discuss the implications first, I'm not quite sure how to define this behavior yet.
The CQ bit was unchecked by hinoka@chromium.org
The CQ bit was checked by hinoka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2196453002/#ps140001 (title: "Reviews")
The CQ bit was checked by hinoka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Milo: Console view prototype This is pretty hacked up. Namely: * This does a 25 build query for _each_ builder, and then filter out the correct revision * This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process) * Hits gitiles for every query (70ms) * Only buildbot is supported * Config is hard-coded instead of being loaded from luci-cfg All of the issues above will be addressed later, this is mostly to get a demo out. BUG=632516 ========== to ========== Milo: Console view prototype This is pretty hacked up. Namely: * This does a 25 build query for _each_ builder, and then filter out the correct revision * This loads the entire build (including un-gzip and json deserialization) just to take like 5 pieces of information from it (~1000ms for the whole process) * Hits gitiles for every query (70ms) * Only buildbot is supported * Config is hard-coded instead of being loaded from luci-cfg All of the issues above will be addressed later, this is mostly to get a demo out. BUG=632516 Committed: https://github.com/luci/luci-go/commit/19b5ff60eca7ed2c7a3f2b71cd1b6ab99d3e04b7 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://github.com/luci/luci-go/commit/19b5ff60eca7ed2c7a3f2b71cd1b6ab99d3e04b7 |
