Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717043002:
Add LogDog log stream fetcher code. (Closed)
DescriptionAdd LogDog log stream fetcher code.
Add the LogDog log stream fetcher TypeScript code. This is a class and
associated facilities capable of asynchronously fetching LogDog log streams
via promises and reporting the fetch status.
BUG=chromium:678673
TEST=None
Review-Url: https://codereview.chromium.org/2717043002
Committed: https://github.com/luci/luci-go/commit/1a272ab20c4b42c9d2a18871a864904d95629fa8
Patch Set 1 #
Total comments: 24
Patch Set 2 : comments, retry "do" #
Total comments: 43
Patch Set 3 : operations, comments #
Total comments: 3
Patch Set 4 : use operation token #
Total comments: 6
Patch Set 5 : add missing client #Patch Set 6 : fix bug, underscore variable names #
Messages
Total messages: 16 (5 generated)
dnj@chromium.org changed reviewers: + hinoka@chromium.org, nodir@chromium.org
PTAL
mostly lg + naming comments Main concern is added complexity with the triply nested retry layer, if we can just get away with one. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:39: * and retrying calls due to: - Transient failures (via RPC client). - Missing newline for bullet points. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:48: * Fetcher offers fetching via "get", "getAll", and "tail". how about "getLast" instead of "tail"? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:80: * If no terminal index is known, or if the log stream is still streaming, "If no terminal index is known because the log is still streaming"? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:89: return (!!(this.lastState && this.lastState.archive)); does bool(...) exist? That would be more readable https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:98: this.currentStatus = st; If these are set together, should they be called either "currentStatus" and "currentError" or "lastStatus" and "lastError", otherwise the naming implies they're set one-off from each other. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:116: * Returns a Promise that resolves to the next block of logs in the stream. "that will resolve to" https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:119: * next block of logs in the stream, or null if there are no logs to "no logs to return currently" or "no more logs to return"? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:167: tail(): Promise<LogDog.LogEntry[]> { getLast()? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:192: // (Testing) Constrain our max logs, if set. Is this actually testing? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:203: let streamingRetry = new luci.RetryIterator(Fetcher.streamingRetry); Why are there 3 layers of retries (I counted 3. the retryIterator(), the recursive call below, and doRetryIfMissing)? Can we just have one? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:231: return tryGet(); Wouldn't this increase the calling stack size on every retry? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:285: private doTail(): Promise<LogDog.LogEntry[]> { doGetLast()?
https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:39: * and retrying calls due to: - Transient failures (via RPC client). - Missing On 2017/02/27 21:50:25, hinoka wrote: > newline for bullet points. Done. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:48: * Fetcher offers fetching via "get", "getAll", and "tail". On 2017/02/27 21:50:25, hinoka wrote: > how about "getLast" instead of "tail"? It's not the last though. Maybe "latest"? https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:80: * If no terminal index is known, or if the log stream is still streaming, On 2017/02/27 21:50:26, hinoka wrote: > "If no terminal index is known because the log is still streaming"? Done. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:89: return (!!(this.lastState && this.lastState.archive)); On 2017/02/27 21:50:25, hinoka wrote: > does bool(...) exist? That would be more readable No :( https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:98: this.currentStatus = st; On 2017/02/27 21:50:25, hinoka wrote: > If these are set together, should they be called either "currentStatus" and > "currentError" or "lastStatus" and "lastError", otherwise the naming implies > they're set one-off from each other. Done. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:116: * Returns a Promise that resolves to the next block of logs in the stream. On 2017/02/27 21:50:25, hinoka wrote: > "that will resolve to" Done. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:119: * next block of logs in the stream, or null if there are no logs to On 2017/02/27 21:50:25, hinoka wrote: > "no logs to return currently" or "no more logs to return"? Pretty sure this can't be null. Updated comment. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:167: tail(): Promise<LogDog.LogEntry[]> { On 2017/02/27 21:50:25, hinoka wrote: > getLast()? getLatest https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:192: // (Testing) Constrain our max logs, if set. On 2017/02/27 21:50:25, hinoka wrote: > Is this actually testing? Used by another module, but yes. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:203: let streamingRetry = new luci.RetryIterator(Fetcher.streamingRetry); On 2017/02/27 21:50:25, hinoka wrote: > Why are there 3 layers of retries (I counted 3. the retryIterator(), the > recursive call below, and doRetryIfMissing)? Can we just have one? There should be two: "retryIterator": streaming: the log exists, and is not terminated, but I haven't gotten all of the things that I want, so try again since there could be more. missing: the user said get this log, but LogDog says 404. I trust the user, this is probably ingest lag, so try again. Streaming backs off slowly to an upper bound until it gets more logs, then resets to try again, backing off slowly. Missing retries until a non-404 is returned. The recursive call isn't a retry. I've simplified this code by: 1) Adding a "do" method to a RetryIterator. 2) Using this everywhere here. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:231: return tryGet(); On 2017/02/27 21:50:25, hinoka wrote: > Wouldn't this increase the calling stack size on every retry? I think so. Simplified via "do", which should not have this problem. https://codereview.chromium.org/2717043002/diff/1/web/inc/logdog-stream-view/... web/inc/logdog-stream-view/fetcher.ts:285: private doTail(): Promise<LogDog.LogEntry[]> { On 2017/02/27 21:50:25, hinoka wrote: > doGetLast()? Since Tail is actually the name of the RPC call, I'm going to leave this as-is.
https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:156: allLogs.push.apply(allLogs, logs); nit, in my experience this is usually written as Array.push.apply(allLogs, logs) https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:160: return getIter(); return resolved promise instead of recursive if count is 0 https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:161: }); then return allLogs? currently allLogs is not used https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:199: if (!opts) { doGet assumes opts is not null. doGet is called below. I recommend doing `opts = opts || {}` before L198 https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:218: return this.doGet(index, opts).then((logs) => { nit: parens around parameters of a lambda expression are optional https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:219: if (!(logs && !logs.length)) { this is hard to read. This is equivalent to if (!logs || logs.length) so looks like this hard-to-read expression hides a bug. Consider changing to if (!logs || !logs.length) https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:234: this.updateStatus(FetcherStatus.STREAMING); all mutations (like updateStatus calls) in callbacks look racy. Imagine that there are two requests in flight. They will race which will set the status. One of them can set the status to IDLE while another is in flight https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:239: .then((logs) => { nit: unnecessary parens https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:242: if (opts.logCount && opts.logCount > 0) { shouldn't this check opts.nonContiguous? currently nonContiguous is never used https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:243: let maxStreamIndex = index + opts.logCount - 1; i don't fully understand this. Can you document logCount? https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:244: logs = logs.filter((le) => { logs = logs.filter(le => le.streamIndex <= maxStreamIndex) https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:271: if (opts) { opts is not falsy since L266 didn't explode https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:335: this.updateStatus(FetcherStatus.IDLE); probably a race https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:337: this.lastDesc = fr.desc; here too https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:354: } return https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:385: let logs = (resp.logs || []).map((le) => { nit let logs = (resp.logs || []).map(le => LogDog.LogEntry.make(le, desc)) fits 80line https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts File web/inc/rpc/client.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:301: * @throws any the error generated by "gen"'s Promise, if out of retries, any error https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:304: do why line break? this causes extra indentation in the function body https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:325: return luci.sleepPromise(delay).then(() => { return luci.sleepPromise(delay).then(() => gen().catch(onErr))
Fixed. PTAL now that I've implemented this via operations. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:156: allLogs.push.apply(allLogs, logs); On 2017/03/08 07:41:28, nodir wrote: > nit, in my experience this is usually written as > Array.push.apply(allLogs, logs) TypeScript complains: Property 'push' does not exist on type 'ArrayConstructor' https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:160: return getIter(); On 2017/03/08 07:41:28, nodir wrote: > return resolved promise instead of recursive if count is 0 Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:161: }); On 2017/03/08 07:41:28, nodir wrote: > then return allLogs? currently allLogs is not used It's used above (see line #144). https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:199: if (!opts) { On 2017/03/08 07:41:28, nodir wrote: > doGet assumes opts is not null. doGet is called below. I recommend doing `opts = > opts || {}` before L198 Actually now that I do strict null checks, I can drop this. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:218: return this.doGet(index, opts).then((logs) => { On 2017/03/08 07:41:28, nodir wrote: > nit: parens around parameters of a lambda expression are optional Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:219: if (!(logs && !logs.length)) { On 2017/03/08 07:41:28, nodir wrote: > this is hard to read. This is equivalent to > > if (!logs || logs.length) > > so looks like this hard-to-read expression hides a bug. > > Consider changing to > > if (!logs || !logs.length) Actually that inner "!" was a bug, it should be: !(logs && logs.length) https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:234: this.updateStatus(FetcherStatus.STREAMING); On 2017/03/08 07:41:28, nodir wrote: > all mutations (like updateStatus calls) in callbacks look racy. Imagine that > there are two requests in flight. They will race which will set the status. One > of them can set the status to IDLE while another is in flight Originally I didn't really care, but I think you're right. I revised everything thusly: 1) An Operation, which has a state and can be cancelled. 2) A Fetch, which binds an Operation to its Promise. All public functions now return Fetch, and all internal operations, as well as the Fetch, expose their Operation. This means that anything can cancel, and state is relative to an individual operation. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:239: .then((logs) => { On 2017/03/08 07:41:28, nodir wrote: > nit: unnecessary parens Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:242: if (opts.logCount && opts.logCount > 0) { On 2017/03/08 07:41:28, nodir wrote: > shouldn't this check opts.nonContiguous? currently nonContiguous is never used Not in this function. Probably should, Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:243: let maxStreamIndex = index + opts.logCount - 1; On 2017/03/08 07:41:28, nodir wrote: > i don't fully understand this. Can you document logCount? Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:244: logs = logs.filter((le) => { On 2017/03/08 07:41:28, nodir wrote: > logs = logs.filter(le => le.streamIndex <= maxStreamIndex) Oh nice, done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:271: if (opts) { On 2017/03/08 07:41:28, nodir wrote: > opts is not falsy since L266 didn't explode Ah yeah, it used to be allowed to be null, but not anymore. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:335: this.updateStatus(FetcherStatus.IDLE); On 2017/03/08 07:41:28, nodir wrote: > probably a race Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:337: this.lastDesc = fr.desc; On 2017/03/08 07:41:28, nodir wrote: > here too Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:354: } On 2017/03/08 07:41:28, nodir wrote: > return Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:385: let logs = (resp.logs || []).map((le) => { On 2017/03/08 07:41:28, nodir wrote: > nit > let logs = (resp.logs || []).map(le => LogDog.LogEntry.make(le, desc)) > fits 80line Done. https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts File web/inc/rpc/client.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:301: * @throws any the error generated by "gen"'s Promise, if out of retries, On 2017/03/08 07:41:28, nodir wrote: > any error "any" is the type https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:304: do On 2017/03/08 07:41:28, nodir wrote: > why line break? > this causes extra indentation in the function body clang-format put it here. https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:325: return luci.sleepPromise(delay).then(() => { On 2017/03/08 07:41:28, nodir wrote: > return luci.sleepPromise(delay).then(() => gen().catch(onErr)) clang-format
PTAL, updated w/ operation token
lgtm https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:156: allLogs.push.apply(allLogs, logs); On 2017/03/08 08:50:14, dnj wrote: > On 2017/03/08 07:41:28, nodir wrote: > > nit, in my experience this is usually written as > > Array.push.apply(allLogs, logs) > > TypeScript complains: Property 'push' does not exist on type 'ArrayConstructor' :( https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:161: }); On 2017/03/08 08:50:14, dnj wrote: > On 2017/03/08 07:41:28, nodir wrote: > > then return allLogs? currently allLogs is not used > > It's used above (see line #144). Acknowledged. https://codereview.chromium.org/2717043002/diff/20001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:234: this.updateStatus(FetcherStatus.STREAMING); On 2017/03/08 08:50:14, dnj wrote: > On 2017/03/08 07:41:28, nodir wrote: > > all mutations (like updateStatus calls) in callbacks look racy. Imagine that > > there are two requests in flight. They will race which will set the status. > One > > of them can set the status to IDLE while another is in flight > > Originally I didn't really care, but I think you're right. I revised everything > thusly: > 1) An Operation, which has a state and can be cancelled. > 2) A Fetch, which binds an Operation to its Promise. > > All public functions now return Fetch, and all internal operations, as well as > the Fetch, expose their Operation. This means that anything can cancel, and > state is relative to an individual operation. great https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts File web/inc/rpc/client.ts (right): https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:301: * @throws any the error generated by "gen"'s Promise, if out of retries, On 2017/03/08 08:50:15, dnj wrote: > On 2017/03/08 07:41:28, nodir wrote: > > any error > > "any" is the type Acknowledged. https://codereview.chromium.org/2717043002/diff/20001/web/inc/rpc/client.ts#n... web/inc/rpc/client.ts:304: do On 2017/03/08 08:50:15, dnj wrote: > On 2017/03/08 07:41:28, nodir wrote: > > why line break? > > this causes extra indentation in the function body > > clang-format put it here. Acknowledged. https://codereview.chromium.org/2717043002/diff/40001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/40001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:54: private cancelledValue = false; nit: _cancelled ? _foo is a typical name when foo is the public API, in TypeScript and C# same applies to the other properties https://codereview.chromium.org/2717043002/diff/40001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:225: } return getIter() https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:66: addStateChangedCallback(cb: (f: Fetch) => void) { add removeStateChangedCallback? https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:108: if (!(st === this.lastStatusValue && err !== this.lastErrorValue)) { err === this.lastErrorValue ? https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:394: this.lastDesc = fr.desc; AFAIU no race here because desc is going to be the same for all requests
lgtm here also
https://codereview.chromium.org/2717043002/diff/40001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/40001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:54: private cancelledValue = false; On 2017/03/13 19:48:22, nodir wrote: > nit: _cancelled > ? > > _foo is a typical name when foo is the public API, in TypeScript and C# > > same applies to the other properties Had to modify tslint rules to allow this, but done. https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... File web/inc/logdog-stream-view/fetcher.ts (right): https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:66: addStateChangedCallback(cb: (f: Fetch) => void) { On 2017/03/13 19:48:22, nodir wrote: > add removeStateChangedCallback? Not necessary, won't ever be called. https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:108: if (!(st === this.lastStatusValue && err !== this.lastErrorValue)) { On 2017/03/13 19:48:22, nodir wrote: > err === this.lastErrorValue > > ? good catch, fixed. https://codereview.chromium.org/2717043002/diff/60001/web/inc/logdog-stream-v... web/inc/logdog-stream-view/fetcher.ts:394: this.lastDesc = fr.desc; On 2017/03/13 19:48:22, nodir wrote: > AFAIU no race here because desc is going to be the same for all requests Correct.
The CQ bit was checked by dnj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org, hinoka@chromium.org Link to the patchset: https://codereview.chromium.org/2717043002/#ps100001 (title: "fix bug, underscore variable names")
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": 100001, "attempt_start_ts": 1489450484647330,
"parent_rev": "918f7ea1301278c62d3dcc5af775f60ff5faf7c2", "commit_rev":
"1a272ab20c4b42c9d2a18871a864904d95629fa8"}
Message was sent while issue was closed.
Description was changed from ========== Add LogDog log stream fetcher code. Add the LogDog log stream fetcher TypeScript code. This is a class and associated facilities capable of asynchronously fetching LogDog log streams via promises and reporting the fetch status. BUG=chromium:678673 TEST=None ========== to ========== Add LogDog log stream fetcher code. Add the LogDog log stream fetcher TypeScript code. This is a class and associated facilities capable of asynchronously fetching LogDog log streams via promises and reporting the fetch status. BUG=chromium:678673 TEST=None Review-Url: https://codereview.chromium.org/2717043002 Committed: https://github.com/luci/luci-go/commit/1a272ab20c4b42c9d2a18871a864904d95629fa8 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://github.com/luci/luci-go/commit/1a272ab20c4b42c9d2a18871a864904d95629fa8 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
