#6530 Improve error handling / reporting on tool importer failure

v1.0.1
closed
Tim Van Steenburgh
import (116) sf-2 (994)
General
Dave Brondsema
2015-08-20
2013-08-07
Cory Johns
No

Currently, if a tool importer fails, the only record of the failure is in the monq_task record. Failures need to be caught and the users notified in some way. A catch-all should be put in place in the import_tool task, but individual tools may want to handle reporting differently, such as the repo clone failure notification that is already in place and we need to ensure that the catch-all doesn't duplicate any per-tool handling.

Partial imports also need to be considered and we might want to decide if a partially failed import is worth rolling back the entire tool import or if the importer should handle a failed ticket, e.g., gracefully and continue to import the rest of the tickets if possible.

Related

Tickets: #6540
Tickets: #6624

Discussion

  • Dave Brondsema

    Dave Brondsema - 2013-08-07
    • Milestone: forge-backlog --> forge-aug-23
     

  • Dave Brondsema

    Dave Brondsema - 2013-08-09
    • Size: --> 2
     

  • Cory Johns - 2013-08-21
    • status: open --> in-progress
    • assigned_to: Cory Johns
     

  • Cory Johns - 2013-08-23

    allura:cj/6530
    forge-classic:cj/6530
    googlecodewikiimporter:cj/6530
    tracwikiimporter:cj/6530

     

  • Cory Johns - 2013-08-23
    • status: in-progress --> code-review
     

  • Dave Brondsema

    Dave Brondsema - 2013-08-23
    • Milestone: forge-aug-23 --> forge-sep-06
     

  • Tim Van Steenburgh - 2013-08-28
    • QA: Tim Van Steenburgh
     

  • Tim Van Steenburgh - 2013-08-28

    I like the way this works, but I think the exception handling should be moved into a contextmanager or a decorator so that it can be easily reused. If you look at [#6526] master you'll notice that all the ToolImporters now have their own @task wrappers, each of which will need this same behavior.

     

    Related

    Tickets: #6526

  • Tim Van Steenburgh - 2013-08-28
    • status: code-review --> in-progress
     

  • Tim Van Steenburgh - 2013-08-29
    • assigned_to: Cory Johns --> Tim Van Steenburgh
    • QA: Tim Van Steenburgh --> nobody
     

  • Tim Van Steenburgh - 2013-08-29

    I'll go ahead and finish this off since Cory is on PTO.

     

  • Tim Van Steenburgh - 2013-08-30
    • status: in-progress --> code-review
     

  • Tim Van Steenburgh - 2013-08-30

    Changes pushed to:

    allura:tv/6530
    forge-classic:tv/6530
    googlecodewikiimporter:tv/6530
    tracwikiimporter:tv/6530

     

  • Dave Brondsema

    Dave Brondsema - 2013-08-30

    Just reading the code real quick (haven't actually tested it, sorry), wouldn't all the import_tool tasks always end up being recorded as 'complete' since the error is caught and passed along to a new task? We had (still have) this same situation with repo_clone tasks, and it makes it misleading when looking at the original task and seeing it's state is "complete" instead of "error". I thought we were going to try to avoid repeating that anti-pattern that in this ticket.

    Using a context manager e.g. https://sourceforge.net/p/allura/pastebin/5220d8170910d42e8a113a68 would be better overall I think, and also allow you to make a single change (like re-raising the exception to address my previous paragraph) in just one place instead of throughout numerous codebases.

     

  • Tim Van Steenburgh - 2013-09-03
    • status: code-review --> in-progress
     

  • Tim Van Steenburgh - 2013-09-03
    • status: in-progress --> code-review
     

  • Tim Van Steenburgh - 2013-09-03

    Refactored error handling into a context manager which posts an event, but allows the exc to propagate up.

    New commits on forge, googlecodewikiimporter, and tracwikiimporter repos.

     

  • Dave Brondsema

    Dave Brondsema - 2013-09-04
    • status: code-review --> closed
    • QA: Dave Brondsema
     

Log in to post a comment.