Conversation
erinleighh
force-pushed
the
tickets/DM-47970
branch
5 times, most recently
from
4573e07 to
8219f87
Compare
erinleighh
force-pushed
the
tickets/DM-47970
branch
2 times, most recently
from
652c375 to
460da8c
Compare
|
Is this still relevant? |
erinleighh
force-pushed
the
tickets/DM-47970
branch
2 times, most recently
from
7858532 to
e4b3329
Compare
erinleighh
force-pushed
the
tickets/DM-47970
branch
from
e4b3329 to
f33a975
Compare
kfindeisen
left a comment
kfindeisen
left a comment
There was a problem hiding this comment.
Please don't double-log exceptions, otherwise looks good.
| ) | ||
| n_raws = len(raws) | ||
| if n_raws == 0: | ||
| _log_trace.debug("No raws to remove for detector %s.", self.visit.detector) |
There was a problem hiding this comment.
I don't think the "for detector" bit really makes sense here -- the point is if we accidentally left some old raws because of a logic bug, they'll get cleaned up too.
| except Exception: | ||
| _log_trace.exception("Raw removal failed for detector %s.", self.visit.detector) | ||
| raise |
There was a problem hiding this comment.
Please do not use log-and-reraise -- exceptions should only be logged once, when they are handled.
If this were a reasonable place to log the exception, it would not make sense to log it at trace level (if something unexpectedly goes wrong, that's at least a warning).
| instrument=self.visit.instrument, | ||
| detector=self.visit.detector, |
There was a problem hiding this comment.
Oops, almost missed this -- please delete all raws unconditionally (maybe with an instrument constraint, but I think the query will resolve just fine without it). That's kind of the point...
2 participants
No description provided.