Summary

Consolidate tool reports into batched reviews.

Review Request #6002 — Created June 16, 2014 and discarded Oct. 6, 2016, 10:14 a.m.

Information

Owner

chipx86

Repository

ReviewBot

Branch

release-0.2.x

Bugs

Depends On

Reviewers

Groups

reviewbot

People

Description

Previously, every tool that ran generated its own review, which could
lead to a large number of potentially useless reviews, depending on
which tools were invoked.

Now, reviews are batched together as much as possible. This is done by
farming off the tool task execution to a new "run_tools" task, which
handles the execution of every tool that will run on the review request,
as well as the publishing of reviews.

This task constructs a ToolMaster, which monitors the status of the tool
tasks. Every 30 seconds, if there are still tools waiting to finish,
ToolMaster will publish what's already been created. This is done until
the last tool has finished, at which point a final publish will occur.

Tools still submit their own review payloads, but those no longer result
in immediately-published reivews. Instead, those are simply staged
reviews, containing state that may eventually be merged into a batched
review. (This state should later move into the database in a nicer form,
instead of re-using Review.)

When the publish occurs, each draft Review that has been created will be
merged together into one main Review, and published.

In order to prevent merging from taking place while a review is being
published (while the diff comments are still being created), the merging
logic will wait until each review it's planning to merge has a
"reviewbot_populated" flag set in the review's extra_data. This is
always set after a draft review from Review Bot is finished.

Testing Done

Added an artificial delay to one tool, and set the batch wait time to 10 seconds.
Published a change that would result in comments on both tools. Got 2 separate
reviews.

Removed the delay and wait time changes, and repeated. Got 1 combined review with
all comments.

Repeated these tests dozens of times while fine-tuning the code, and consistently
saw the expected behavior.

(There was one time that tool execution stalled, but this was before any of the new
special stuff ran, and this problem went away after upgrading Celery. Still, worth
noting.)

Issues

Description	From	Last Updated
'time' imported but unused	reviewbot	June 16, 2014, 4:32 p.m.
Col: 13 E129 visually indented line with same indent as next logical line	reviewbot	June 16, 2014, 4:32 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	June 16, 2014, 4:32 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	June 16, 2014, 4:33 p.m.
Col: 9 E128 continuation line under-indented for visual indent	reviewbot	June 16, 2014, 4:33 p.m.
Col: 36 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:34 p.m.
Col: 39 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:34 p.m.
Col: 35 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:34 p.m.
Col: 21 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:34 p.m.
Col: 11 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:34 p.m.
Instead of "a specified number of seconds", how about "PUBLISH_WAIT_TIME_SECS seconds"?	david	May 5, 2015, 2:16 p.m.
This seems more like a debug log level sort of message.	david	July 31, 2014, 4:03 p.m.
Also seems debug-ish.	david	July 31, 2014, 4:03 p.m.
Col: 36 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:41 p.m.
Col: 39 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:41 p.m.
Col: 35 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:41 p.m.
Col: 21 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:41 p.m.
Col: 11 E126 continuation line over-indented for hanging indent	reviewbot	June 16, 2014, 4:41 p.m.
Indentation too deep.	david	July 31, 2014, 4:03 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    extension/reviewbotext/resources.py
    bot/reviewbot/celery.py
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    extension/reviewbotext/resources.py
    bot/reviewbot/celery.py
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
  Ignored Files:

Change Summary:

Nothing changed here. Just hoping the new Review Bot works :)

Diff:

Revision 2 (+276 -64)

Show changes

	bot/reviewbot/celery.py
	bot/reviewbot/tasks.py
	bot/reviewbot/tools/__init__.py
	extension/reviewbotext/extension.py
	extension/reviewbotext/resources.py

Change Summary:

Require Celery 3.1.12, to (I think) prevent issues I was hitting.

Commit:

9001a947558cdeda0a55a180128a0e1861f30cb1

e817730b2d667c4f21ac5e743dc88428de61f055

Diff:

Revision 3 (+277 -65)

Show changes

	bot/reviewbot/celery.py
	bot/reviewbot/tasks.py
	bot/reviewbot/tools/__init__.py
	extension/setup.py
	extension/reviewbotext/extension.py
	extension/reviewbotext/resources.py

Change Summary:

Replaced the while loop, which caused some deadlock issues, with periodic timers.

Nice thing about these is that they'll actually attempt to resume if the workers are restarted.

Commit:

e817730b2d667c4f21ac5e743dc88428de61f055

fe203aa8a5bcfbebaeedcc4872e423844f72c793

Diff:

Revision 4 (+283 -65)

Show changes

	bot/reviewbot/celery.py
	bot/reviewbot/tasks.py
	bot/reviewbot/tools/__init__.py
	extension/setup.py
	extension/reviewbotext/extension.py
	extension/reviewbotext/resources.py

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py



Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py

bot/reviewbot/tasks.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 'time' imported but unused
```

bot/reviewbot/tasks.py (Diff revision 4)

The issue has been dropped. Show all issues

Col: 13
 E129 visually indented line with same indent as next logical line

bot/reviewbot/tasks.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
bot/reviewbot/tasks.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
bot/reviewbot/tasks.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 9
 E128 continuation line under-indented for visual indent
```
extension/reviewbotext/extension.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 36
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 39
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 35
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 21
 E126 continuation line over-indented for hanging indent
```
extension/setup.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 11
 E126 continuation line over-indented for hanging indent
```

Change Summary:



Fixed a bunch of the open issues reported by Review Bot (thanks buddy!).

  Didn't fix the ones on unrelated lines.


Fixed an issue with tool settings getting overwritten, due to reuse of the tool_payload dictionary.

Commit:

fe203aa8a5bcfbebaeedcc4872e423844f72c793

a47c3ccd7c1a8266b578a0b45b9cfcdef883bf13

Diff:

Revision 5 (+284 -66)

Show changes

	bot/reviewbot/celery.py
	bot/reviewbot/tasks.py
	bot/reviewbot/tools/__init__.py
	extension/setup.py
	extension/reviewbotext/extension.py
	extension/reviewbotext/resources.py

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py



Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py

extension/reviewbotext/extension.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 36
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 39
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 35
 E126 continuation line over-indented for hanging indent
```
extension/reviewbotext/resources.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 21
 E126 continuation line over-indented for hanging indent
```
extension/setup.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 11
 E126 continuation line over-indented for hanging indent
```

bot/reviewbot/tasks.py (Diff revision 5)

The issue has been resolved. Show all issues

Instead of "a specified number of seconds", how about "PUBLISH_WAIT_TIME_SECS seconds"?

bot/reviewbot/tasks.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This seems more like a debug log level sort of message.
```
bot/reviewbot/tasks.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Also seems debug-ish.
```
extension/setup.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Indentation too deep.
```

Change Summary:


Switched some info logging to debug.
Updated a docstring to reference a constant.
Fixed an over-indentation.
Fixed a crash due to indexing into an empty list of reviews (which can happen, for instance, when the review request applies to a repository ReviewBot doesn't have access to).

Commit:

a47c3ccd7c1a8266b578a0b45b9cfcdef883bf13

a135b1b8109bd8e193c8851321322c28a91111af

Diff:

Revision 6 (+286 -66)

Show changes

	bot/reviewbot/celery.py
	bot/reviewbot/tasks.py
	bot/reviewbot/tools/__init__.py
	extension/setup.py
	extension/reviewbotext/extension.py
	extension/reviewbotext/resources.py

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py



Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    extension/reviewbotext/extension.py
    bot/reviewbot/tools/__init__.py
    extension/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/resources.py

Ship it!

I'm not a reviewbot expert but I don't see anything obviously wrong.

This is no longer necessary with the Review Bot rework I'm doing.

Status:: Discarded