Code review comment for lp:~diogobaeder/storm/cleanup-timeline

Revision history for this message
Sidnei da Silva (sidnei) wrote :

There's an alternative failure scenario that this fix doesn't cover:

  - If the cursor.execute() raises a DatabaseError, the PostgresTimeoutTracer's connection_raw_execute_error may turn that into a TimeoutError and re-raise.
  - If the TimelineTracer (or any other tracer for that matter) comes last, then it's connection_raw_execute_error will never be called because the exception bubbles up from trace(), never notifying the remaining tracers.

An additional solution to that problem might be to also make the trace() function more robust, ensuring that it always goes through all the tracers and collects all exceptions, raising some sort of MultiException if there is more than one.

The change of behaviour *could* break people relying on the broken behaviour (eg, expecting that if one of the tracers fails, the remaining ones will never execute), but I don't think anyone in their sane mind would do that.

« Back to merge proposal