Merge lp:~cprov/adt-result-checker/missing-ack-on-retry into lp:adt-result-checker

[Celso Providelo]

Adding missing message acknowledge when tests requests have to be retried.

Adding a missing message acknowledge when the test request had to be retried.

Code flow and readability in result-checker is not good and unfortunately it is the microservice more likely to change/evolve to provide better user experience (IMO).

I think it's related with the complete lack of unittests and Thomi has made his point here (biased ? extrapolated ? maybe ...), that we do need some extent of unittest in order to be comfortable enough with bug-fixing and small refactoring.

[Para Siva (psivaa)]

LGTM

review: Approve

[Paul Larson (pwlars)]

should the message.ack() move to maybe_retry_message? It seems that there are some circumstances in maybe_retry_message where it could already get acked by way of insert_into_dead_letter_queue()

review: Needs Information

[lp:~cprov/adt-result-checker/missing-ack-on-retry]

6. By Celso Providelo on 2015-03-19

Stop double-acking when the message goes to the dead-letter queue.

[Paul Larson (pwlars)]

+1

review: Approve

[Celso Providelo (cprov)]

I will work on basic unittests for a worker (kombu.ConsumerMixin) in a separated branch, we can use it.

=== modified file '.bzrignore'
--- .bzrignore	2015-03-16 02:06:27 +0000
+++ .bzrignore	2015-03-19 13:09:13 +0000
@@ -1,1 +1,2 @@
 ve
+adt_result_checker.egg-info

=== modified file 'adt_result_checker/__init__.py'
--- adt_result_checker/__init__.py	2015-03-18 01:58:38 +0000
+++ adt_result_checker/__init__.py	2015-03-19 13:09:13 +0000
@@ -73,18 +73,19 @@
                 message,
                 "Invalid result message format: {}".format(str(e))
             )
+            message.ack()
         else:
             if exit_code_implies_retry(exit_code):
                 self.maybe_retry_message(message)
+                message.ack()
             else:
                 if result_tarball_exists_in_swift(request_id):
-                    if self.post_results(request_id):
-                        message.ack()
-                    else:
+                    if not self.post_results(request_id):
                         self.maybe_retry_message(
                             message,
                             "Unable to post results to swift"
                         )
+                    message.ack()
                 else:
                     # Swift does not yet contain the result tarball, so we
                     # can't process this yet: put it back in the queue for
@@ -106,7 +107,6 @@
         # own simple queue that no one ever automatically reads from. This is
         # why we ack this message, even though we're really failing it.
         message.payload['deadletter_reason'] = reason
-        message.ack()
 
         queue = self.connection.SimpleQueue('adt.deadletters.{}'.format(API_VERSION))
         queue.put(message.payload)