Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(683)

Issue 112320044: Don't calculate deltas when viewing issues because that's done in a task (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 4 months ago by jrobbins (chromium)
Modified:
3 months, 3 weeks ago
CC:
codereview-list_groups.google.com
Visibility:
Public.

Description

When an upload completes, it kicks off a task to calculate deltas. So, there is no reason to do the same work when viewing the code review issue. Since calculating deltas is done in a task, it has a deadline of 10 minutes and automatically retries, so there is no need to implement our own retry logic or to display an error message to the user. Calculating deltas while the base files were still being uploaded (because the user views the issue while still uploading) caused a race condition that over-wrote the patch in a non-transactional way. Delta calculation in a task does not have that risk because the task is only enqueue after the upload is complete. BUG=https://code.google.com/p/rietveld/issues/detail?id=357

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -80 lines) Patch
M codereview/models.py View 2 chunks +1 line, -26 lines 0 comments Download
M codereview/views.py View 5 chunks +10 lines, -54 lines 0 comments Download

Messages

Total messages: 10
jrobbins (corp)
PTAL. This should avoid some corrupted code review issues.
7 years, 4 months ago (2014-07-15 19:58:35 UTC) #1
iannucci
lgtm. I assume that this means that the delta links will just not be visible ...
7 years, 4 months ago (2014-07-17 18:37:22 UTC) #2
jrobbins (corp)
On 2014/07/17 18:37:22, iannucci wrote: > lgtm. I assume that this means that the delta ...
7 years, 4 months ago (2014-07-17 18:56:38 UTC) #3
iannucci
On 2014/07/17 18:56:38, jrobbins (corp) wrote: > On 2014/07/17 18:37:22, iannucci wrote: > > lgtm. ...
7 years, 4 months ago (2014-07-17 19:02:26 UTC) #4
jrobbins (corp)
Committed as https://code.google.com/p/rietveld/source/detail?r=bdb32f6d8b0922d3c417659e271bad439a8ee55c
7 years, 4 months ago (2014-07-17 19:20:06 UTC) #5
ajaramillo1629
6 years, 5 months ago (2015-06-05 00:13:16 UTC) #6
mcourtney1961
On 2014/07/15 19:58:35, jrobbins (corp) wrote: > PTAL. This should avoid some corrupted code review ...
3 years, 5 months ago (2018-06-10 21:39:14 UTC) #7
mcourtney1961
On 2014/07/17 18:37:22, iannucci wrote: > lgtm. I assume that this means that the delta ...
3 years, 5 months ago (2018-06-10 21:40:29 UTC) #8
lehmanpstrik
On 2018/06/10 21:40:29, mcourtney1961 wrote: > On 2014/07/17 18:37:22, iannucci wrote: > > lgtm. I ...
4 months, 3 weeks ago (2021-07-07 00:41:22 UTC) #9
lehmanpstrik
3 months, 3 weeks ago (2021-08-06 02:29:37 UTC) #10
Shop are u


Holen Sie sich Outlook für Android<https://aka.ms/AAb9ysg>
________________________________
From: lehmanpstrik@gmail.com <lehmanpstrik@gmail.com>
Sent: Wednesday, July 7, 2021 1:41:22 AM
To: jrobbins@chromium.org <jrobbins@chromium.org>; iannucci@chromium.org
<iannucci@chromium.org>; jrobbins@google.com <jrobbins@google.com>;
ajaramillo1629@gmail.com <ajaramillo1629@gmail.com>; mcourtney1961@yahoo.com
<mcourtney1961@yahoo.com>
Cc: codereview-list@groups.google.com <codereview-list@groups.google.com>;
reply@codereview-hr.appspotmail.com <reply@codereview-hr.appspotmail.com>
Subject: Re: Don't calculate deltas when viewing issues because that's done in a
task (issue 112320044 by jrobbins@chromium.org)

On 2018/06/10 21:40:29, mcourtney1961 wrote:
> On 2014/07/17 18:37:22, iannucci wrote:
> > lgtm. I assume that this means that the delta links will just not be
visible
> > until the task queue task completes?
> >
> > Would it be better to have a cron job which just looks for all
Patchsets which
> > have deltas_calculated=False? Then the immediate task queue is an
optimization
> > over the cron job, but the system will stabilize towards a good
state.
>
> Ifk!



https://codereview.appspot.com/112320044/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b