• 
      

    Consolidate tool reports into batched reviews.

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

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