launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/lazr.jobrunner/quieter-retry-handling into lp:lazr.jobrunner.

Commit message:
Reduce "Scheduling retry" log message to INFO.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lazr.jobrunner/quieter-retry-handling/+merge/369697

Messages at WARNING and above trigger OOPSes in Launchpad, and this isn't really a warning anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/lazr.jobrunner/quieter-retry-handling into lp:lazr.jobrunner.

=== modified file 'NEWS.txt'
--- NEWS.txt	2019-05-13 15:06:24 +0000
+++ NEWS.txt	2019-07-04 10:52:01 +0000
@@ -1,6 +1,12 @@
 News
 ====
 
+UNRELEASED
+----------
+* Reduce "Scheduling retry" log message to INFO, since messages at WARNING
+  and above trigger OOPSes in Launchpad and this isn't really a warning
+  anyway.
+
 0.15
 ----
 * Add Python 3 support (requires celery >= 4.3).

=== modified file 'src/lazr/jobrunner/jobrunner.py'
--- src/lazr/jobrunner/jobrunner.py	2019-04-17 11:35:55 +0000
+++ src/lazr/jobrunner/jobrunner.py	2019-07-04 10:52:01 +0000
@@ -163,7 +163,7 @@
             except self.retryErrorTypes(job) as e:
                 if job.attempt_count > job.max_retries:
                     raise
-                self.logger.warning(
+                self.logger.info(
                     "Scheduling retry due to %s: %s." % (
                         e.__class__.__name__, e))
                 job.queue(manage_transaction=True)

=== modified file 'src/lazr/jobrunner/tests/test_jobrunner.py'
--- src/lazr/jobrunner/tests/test_jobrunner.py	2019-03-05 20:26:00 +0000
+++ src/lazr/jobrunner/tests/test_jobrunner.py	2019-07-04 10:52:01 +0000
@@ -18,6 +18,7 @@
 
 import contextlib
 import logging
+import sys
 from unittest import TestCase
 
 import oops
@@ -134,6 +135,30 @@
         return [oops_id]
 
 
+class OopsHandler(logging.Handler):
+    """Handler to log WARNING and above to the OOPS system.
+
+    This isn't an essential part of lazr.jobrunner, but Launchpad uses
+    something similar, so we borrow it so that we can test that we don't
+    trigger OOPSes there.
+    """
+
+    def __init__(self, oops_config, level=logging.WARNING):
+        super(OopsHandler, self).__init__(level)
+        self.oops_config = oops_config
+
+    def emit(self, record):
+        """Emit a record as an OOPS."""
+        try:
+            info = record.exc_info
+            if info is None:
+                info = sys.exc_info()
+            report = self.oops_config.create(context={'exc_info': info})
+            self.oops_config.publish(report)
+        except Exception:
+            self.handleError(record)
+
+
 class TestJobRunner(TestCase):
 
     def setUp(self):
@@ -246,16 +271,35 @@
             'Job suspended itself',
             self.log_handler.records[-1].msg)
 
+    def test_runJobHandleError_RetryError(self):
+        # If a job raises an exception that is declared as a retry error,
+        # an OOPS is not created.
+        class TryAgain(Exception):
+            pass
+
+        self.logger.addHandler(OopsHandler(self.oops_config))
+        job = FakeJob(1, TryAgain('no OOPS expected'))
+        job.retry_error_types = (TryAgain, )
+        job.max_retries = 1
+        self.runner.runJobHandleError(job)
+        self.assertFalse(job.notifyOops_called)
+        self.assertEqual(0, self.oops_repository.oops_count)
+        self.assertEqual(
+            "Scheduling retry due to TryAgain: no OOPS expected.",
+            self.log_handler.records[-1].msg)
+
     def test_runJobHandleError_UserError(self):
         # If a job raises an exception that is declared as a user error,
         # an OOPS is not created.
         class UserError(Exception):
             pass
 
+        self.logger.addHandler(OopsHandler(self.oops_config))
         job = FakeJob(1, UserError('no OOPS expected'))
         job.user_error_types = (UserError, )
         self.assertFalse(job.notifyUserError_called)
         self.runner.runJobHandleError(job)
+        self.assertFalse(job.notifyOops_called)
         self.assertEqual(0, self.oops_repository.oops_count)
         self.assertEqual(
             "<FakeJob> (ID 1) failed with user error "