Consolidate tool reports into batched reviews.

Review Request #6002 — Created June 16, 2014 and discarded

Information

ReviewBot
release-0.2.x

Reviewers

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.

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.)

Description From Last Updated

'time' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 36 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 39 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 35 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 11 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

daviddavid

This seems more like a debug log level sort of message.

daviddavid

Also seems debug-ish.

daviddavid

Col: 36 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 39 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 35 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 11 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Indentation too deep.

daviddavid
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. 
      
chipx86
chipx86
chipx86
reviewbot
  1. 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
    
    
  2. bot/reviewbot/tasks.py (Diff revision 4)
     
     
     'time' imported but unused
    
  3. bot/reviewbot/tasks.py (Diff revision 4)
     
     
    Col: 13
     E129 visually indented line with same indent as next logical line
    
  4. bot/reviewbot/tasks.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  5. bot/reviewbot/tasks.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. bot/reviewbot/tasks.py (Diff revision 4)
     
     
    Col: 9
     E128 continuation line under-indented for visual indent
    
  7. extension/reviewbotext/extension.py (Diff revision 4)
     
     
    Col: 36
     E126 continuation line over-indented for hanging indent
    
  8. extension/reviewbotext/resources.py (Diff revision 4)
     
     
    Col: 39
     E126 continuation line over-indented for hanging indent
    
  9. extension/reviewbotext/resources.py (Diff revision 4)
     
     
    Col: 35
     E126 continuation line over-indented for hanging indent
    
  10. extension/reviewbotext/resources.py (Diff revision 4)
     
     
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  11. extension/setup.py (Diff revision 4)
     
     
    Col: 11
     E126 continuation line over-indented for hanging indent
    
  12. 
      
chipx86
reviewbot
  1. 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
    
    
  2. extension/reviewbotext/extension.py (Diff revision 5)
     
     
    Col: 36
     E126 continuation line over-indented for hanging indent
    
  3. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 39
     E126 continuation line over-indented for hanging indent
    
  4. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 35
     E126 continuation line over-indented for hanging indent
    
  5. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  6. extension/setup.py (Diff revision 5)
     
     
    Col: 11
     E126 continuation line over-indented for hanging indent
    
  7. 
      
david
  1. 
      
  2. bot/reviewbot/tasks.py (Diff revision 5)
     
     

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

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

    This seems more like a debug log level sort of message.

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

    Also seems debug-ish.

  5. extension/setup.py (Diff revision 5)
     
     

    Indentation too deep.

  6. 
      
chipx86
reviewbot
  1. 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
    
    
  2. 
      
david
  1. I'm not a reviewbot expert but I don't see anything obviously wrong.

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

  2. 
      
david
Review request changed

Status: Discarded

Loading...