Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796743004:
Milo flex raw log viewer endpoint (Closed)
|
Created:
3 years, 8 months ago by Ryan Tseng Modified:
3 years, 7 months ago CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionMilo flex raw log viewer endpoint
This is a simple flex app that runs the logdog fetcher and serves back log text. It runs in flex to avoid the GAE classic restrictions such as response size and response time.
Most of the logic is setting up a flex environment, most notably, memcache is not yet available. The idea is to refactor this to look more like the GAE Classic middleware once a public memcache API is released. Until then, memcache is simply not used.
BUG=698429
Review-Url: https://codereview.chromium.org/2796743004
Committed: https://github.com/luci/luci-go/commit/d7caba160073c8b83e1b7d393227162f910d46ee
Patch Set 1 #Patch Set 2 : Refactor for middleware #Patch Set 3 : Cleaning up, add logs.go #Patch Set 4 : Tidy up #Patch Set 5 : Remove debug prints #Patch Set 6 : Fix makefile #Patch Set 7 : Remove more debug comments #
Total comments: 1
Patch Set 8 : Remove gaemiddleware/context.go" #Patch Set 9 : GoImports #
Total comments: 16
Patch Set 10 : Review + Working #Patch Set 11 : Rebase #Patch Set 12 : SetConfig fix #Patch Set 13 : Update with UseFlex() #Patch Set 14 : Fix setConfig #Patch Set 15 : Cleanup #Patch Set 16 : Rip out auth #Patch Set 17 : Fix module path #
Total comments: 30
Patch Set 18 : review #Patch Set 19 : More review #
Messages
Total messages: 95 (81 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...
Description was changed from ========== Milo flex log viewer proof of concept DO NOT COMMIT BUG= ========== to ========== Milo flex log viewer proof of concept DO NOT COMMIT DO NOT COMMIT BUG= ==========
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/3554700cad9d2010)
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 Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/355e580661d9cf10)
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 Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/355e8ae32dcc1210)
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 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 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...
Description was changed from ========== Milo flex log viewer proof of concept DO NOT COMMIT DO NOT COMMIT BUG= ========== to ========== Milo flex raw log viewer endpoint This is a simple flex app that runs the logdog fetcher and serves back log text. It runs in flex to avoid the GAE classic restrictions such as response size and response time. Most of the logic is setting up a flex environment, most notably, memcache is not yet available. The idea is to refactor this to look more like the GAE Classic middleware once a public memcache API is released. Until then, memcache is simply not used. BUG=698429 ==========
hinoka@chromium.org changed reviewers: + nodir@chromium.org, vadimsh@chromium.org
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: Try jobs failed on following builders: Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/359701a860814010)
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/35970a0d3e5ea410)
hinoka@chromium.org changed reviewers: + hinoka@chromium.org
I just want to land this so that it's there and available for debug usage and A/B comparisons. Its not going to be in a critical path atm. +Vadim ptal at middleware.go:FlexBase() +Nodir overall review. https://codereview.chromium.org/2796743004/diff/120001/appengine/gaemiddlewar... File appengine/gaemiddleware/context.go (right): https://codereview.chromium.org/2796743004/diff/120001/appengine/gaemiddlewar... appengine/gaemiddleware/context.go:78: project := os.Getenv("GCLOUD_PROJECT") This assumes we are in a GCE container.
https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... File milo/appengine/common/middleware.go (right): https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:71: &server.OAuth2Method{Scopes: []string{server.EmailScope}}, fyi: this doesn't work on Flex currently https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:73: &server.InboundAppIDAuthMethod{}, nit: drop this, we don't actually use it, and it won't work on Flex (because it uses magic GAE headers) https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:86: // TODO(hinoka): Use the cloud logger, somehow. There's this: https://github.com/luci/luci-go/blob/master/common/logging/cloudlog/logging.g... I believe some logdog guts use it. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:93: c.Context = memory.UseWithAppID(c.Context, project) hm... This looks dangerous. Most services there are mocks. Dangerous, because instead of failing, the code may do slightly wrong thing. For example, memcache is assumed to be globally consistent. With impl/memory implementation, it is consistent only within the scope of one process. It may break code that makes assumptions about global nature of memcache. For example, we use memcache-based global locks in some places. A less dangerous approach would be to add only service that really work, and carefully refactor code that depends on other services (like memcache) to not depend on them (somehow). https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:118: DBProvider: authdb.NewDBCache(server.GetAuthDB), 'config' variable should be global. It is stateful, in particular DBProvider and Cache fields point to caches. Currently each invocation of FlexBase() will generate new copy of all these caches, this is not good. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:119: Signer: gaesigner.Signer{}, fyi: this will not work, but milo isn't signing anything, so it's fine. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:124: Cache: auth.MemoryCache(10), nit: 1000 or something we have gigs of ram on Flex IIUC :) https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:132: logger, memory, ds, secrets, auth.Use(authMethods()), authConfig, auth.Authenticate) I think authConfig should be before auth.Use(...)
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 Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a17ce3cde9fd10) Luci-go Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a17ce3d3c17c10)
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 Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a183ec8f5f9210) Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a183eca3407b10) Luci-go Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a183ec60275410)
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 Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a18a274ed1a910) Luci-go Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a18a2771adde10)
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 Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a6662edb458310)
The CQ bit was checked by hinoka@chromium.org 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 Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a681230a424310)
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 Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/35a6a87880d37d10)
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.
ptal again. Uploaded and seems to work: https://logs-dot-luci-milo-dev.appspot.com/log/raw/chromium/bb/tryserver.chro... https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... File milo/appengine/common/middleware.go (right): https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:71: &server.OAuth2Method{Scopes: []string{server.EmailScope}}, On 2017/04/18 00:33:24, Vadim Sh. wrote: > fyi: this doesn't work on Flex currently I'll wait to rebase https://codereview.chromium.org/2830443003/ in https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:73: &server.InboundAppIDAuthMethod{}, On 2017/04/18 00:33:24, Vadim Sh. wrote: > nit: drop this, we don't actually use it, and it won't work on Flex (because it > uses magic GAE headers) Same as above https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:86: // TODO(hinoka): Use the cloud logger, somehow. On 2017/04/18 00:33:24, Vadim Sh. wrote: > There's this: > https://github.com/luci/luci-go/blob/master/common/logging/cloudlog/logging.g... > > I believe some logdog guts use it. Done. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:93: c.Context = memory.UseWithAppID(c.Context, project) On 2017/04/18 00:33:24, Vadim Sh. wrote: > hm... This looks dangerous. Most services there are mocks. Dangerous, because > instead of failing, the code may do slightly wrong thing. For example, memcache > is assumed to be globally consistent. With impl/memory implementation, it is > consistent only within the scope of one process. It may break code that makes > assumptions about global nature of memcache. For example, we use memcache-based > global locks in some places. > > A less dangerous approach would be to add only service that really work, and > carefully refactor code that depends on other services (like memcache) to not > depend on them (somehow). Addressing in https://codereview.chromium.org/2829903002 (Need to roll that in first before landing this) https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:118: DBProvider: authdb.NewDBCache(server.GetAuthDB), On 2017/04/18 00:33:24, Vadim Sh. wrote: > 'config' variable should be global. It is stateful, in particular DBProvider and > Cache fields point to caches. > > Currently each invocation of FlexBase() will generate new copy of all these > caches, this is not good. Moved to init() https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:119: Signer: gaesigner.Signer{}, On 2017/04/18 00:33:24, Vadim Sh. wrote: > fyi: this will not work, but milo isn't signing anything, so it's fine. Acknowledged. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:124: Cache: auth.MemoryCache(10), On 2017/04/18 00:33:24, Vadim Sh. wrote: > nit: 1000 or something > we have gigs of ram on Flex IIUC :) Done. https://codereview.chromium.org/2796743004/diff/160001/milo/appengine/common/... milo/appengine/common/middleware.go:132: logger, memory, ds, secrets, auth.Use(authMethods()), authConfig, auth.Authenticate) On 2017/04/18 00:33:24, Vadim Sh. wrote: > I think authConfig should be before auth.Use(...) Done.
vadimsh: Given you're removing cookie auth, would it be better to rip that out for now?
On 2017/04/24 23:04:38, hinoka wrote: > vadimsh: Given you're removing cookie auth, would it be better to rip that out > for now? rip what out exactly?
(Regarding this CL) Remove all of the middlewares except for logging and base.
On 2017/04/24 23:23:18, hinoka wrote: > (Regarding this CL) Remove all of the middlewares except for logging and base. And make sure it can display only already publicly visible logs?
Yeah, and then make sure it makes logdog requests anonymously only
On 2017/04/24 23:26:49, hinoka wrote: > Yeah, and then make sure it makes logdog requests anonymously only Okay.
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 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...
ptal latest patch. Default http roundtripper is used so everything is unauthenticated. Demo still works: https://logs-dot-luci-milo-dev.appspot.com/log/raw/chromium/bb/tryserver.chro...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm to land it https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... File milo/appengine/common/middleware.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:74: // Flex returns the basic middleware for use on appengine flex. Flex does not s/Flex/FlexBase/ https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:90: http.DefaultClient) i doubt cloud logging API will accept anonymous requests https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:94: cloudlog.Use(c.Context, cloudlog.Config{}, logClient) this is noop because it misses c.Context = ... possibly all stdout or stderr is automatically sent to cloud logging https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:103: return router.NewMiddlewareChain(logger, base) why logger comes before base? if you plan to log using cloudlogging, you may need app service account which is probably accessibly only after base is initialized https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... File milo/appengine/logs/logs.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:21: "golang.org/x/net/context" 3p imports should be before 1p https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:50: s.Lock() FWIU, this serializes all requests to logdog. It is probably fine for the first implementaiton https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:62: reqStream := (s.streamState == nil || s.streamState.State.TerminalIndex < 0) nit: parens are unnecessary https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:64: params = append(params, coordinator.WithState(&streamState)) it seems that this variable controls whether or not to request state. Maybe rename it to reqState? https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:86: if err != nil { this is constant true because of `case nil` above, so the condition can be omitted https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:100: streamPath := types.StreamPath(spath[1]) this will crash the server if someone requests /logs/raw/x https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:104: // TODO(hinoka): Once 712506 is resolved, figure out how to get auth. please write crbug.com/712506 https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:112: src := coordinatorSource{ nit: consider inlining https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:133: break // We're done. i think this breaks the switch, not the loop you can do `break loop` here and add `loop:` to L126 https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... File milo/appengine/logs/main.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... milo/appengine/logs/main.go:31: log.Fatal(http.ListenAndServe(":8080", nil)) why are we unconditionally failing? i think this should call Fatal only if ListenAndServe returned an error https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... milo/appengine/logs/main.go:42: errorPage(c, "missing path") this should be http 400, not 500
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...
https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... File milo/appengine/common/middleware.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:74: // Flex returns the basic middleware for use on appengine flex. Flex does not On 2017/04/29 00:59:44, nodir wrote: > s/Flex/FlexBase/ Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:90: http.DefaultClient) On 2017/04/29 00:59:44, nodir wrote: > i doubt cloud logging API will accept anonymous requests Acknowledged. This probably doesn't work, adding TODO, since all thats required is to swap out the roundtripper. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:94: cloudlog.Use(c.Context, cloudlog.Config{}, logClient) On 2017/04/29 00:59:45, nodir wrote: > this is noop because it misses c.Context = ... > > possibly all stdout or stderr is automatically sent to cloud logging Oh you're right.. that explains a lot. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/common/... milo/appengine/common/middleware.go:103: return router.NewMiddlewareChain(logger, base) On 2017/04/29 00:59:44, nodir wrote: > why logger comes before base? if you plan to log using cloudlogging, you may > need app service account which is probably accessibly only after base is > initialized Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... File milo/appengine/logs/logs.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:21: "golang.org/x/net/context" On 2017/04/29 00:59:45, nodir wrote: > 3p imports should be before 1p Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:50: s.Lock() On 2017/04/29 00:59:45, nodir wrote: > FWIU, this serializes all requests to logdog. It is probably fine for the first > implementaiton Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:62: reqStream := (s.streamState == nil || s.streamState.State.TerminalIndex < 0) On 2017/04/29 00:59:45, nodir wrote: > nit: parens are unnecessary Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:64: params = append(params, coordinator.WithState(&streamState)) On 2017/04/29 00:59:45, nodir wrote: > it seems that this variable controls whether or not to request state. Maybe > rename it to reqState? Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:86: if err != nil { On 2017/04/29 00:59:45, nodir wrote: > this is constant true because of `case nil` above, so the condition can be > omitted Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:100: streamPath := types.StreamPath(spath[1]) On 2017/04/29 00:59:45, nodir wrote: > this will crash the server if someone requests /logs/raw/x Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:104: // TODO(hinoka): Once 712506 is resolved, figure out how to get auth. On 2017/04/29 00:59:45, nodir wrote: > please write crbug.com/712506 Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:112: src := coordinatorSource{ On 2017/04/29 00:59:45, nodir wrote: > nit: consider inlining Done. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/lo... milo/appengine/logs/logs.go:133: break // We're done. On 2017/04/29 00:59:45, nodir wrote: > i think this breaks the switch, not the loop > you can do `break loop` here and add `loop:` to L126 Oh you're right... I think "return nil" will work too. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... File milo/appengine/logs/main.go (right): https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... milo/appengine/logs/main.go:31: log.Fatal(http.ListenAndServe(":8080", nil)) On 2017/04/29 00:59:45, nodir wrote: > why are we unconditionally failing? > i think this should call Fatal only if ListenAndServe returned an error This was how the sample code did it for the appenigne flex example, so I went along with it. I think the theory is that ListenAndServe should never return anything since it's an infinite loop, and if it does, it's fatal. https://codereview.chromium.org/2796743004/diff/310001/milo/appengine/logs/ma... milo/appengine/logs/main.go:42: errorPage(c, "missing path") On 2017/04/29 00:59:45, nodir wrote: > this should be http 400, not 500 Done.
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/2796743004/#ps350001 (title: "More review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1493680098624100,
"parent_rev": "9e23ca809ad15dd07280b8bfa93fdc132d081b16", "commit_rev":
"d7caba160073c8b83e1b7d393227162f910d46ee"}
Message was sent while issue was closed.
Description was changed from ========== Milo flex raw log viewer endpoint This is a simple flex app that runs the logdog fetcher and serves back log text. It runs in flex to avoid the GAE classic restrictions such as response size and response time. Most of the logic is setting up a flex environment, most notably, memcache is not yet available. The idea is to refactor this to look more like the GAE Classic middleware once a public memcache API is released. Until then, memcache is simply not used. BUG=698429 ========== to ========== Milo flex raw log viewer endpoint This is a simple flex app that runs the logdog fetcher and serves back log text. It runs in flex to avoid the GAE classic restrictions such as response size and response time. Most of the logic is setting up a flex environment, most notably, memcache is not yet available. The idea is to refactor this to look more like the GAE Classic middleware once a public memcache API is released. Until then, memcache is simply not used. BUG=698429 Review-Url: https://codereview.chromium.org/2796743004 Committed: https://github.com/luci/luci-go/commit/d7caba160073c8b83e1b7d393227162f910d46ee ==========
Message was sent while issue was closed.
Committed patchset #19 (id:350001) as https://github.com/luci/luci-go/commit/d7caba160073c8b83e1b7d393227162f910d46ee |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
