Code review comment for lp:~al-maisan/landscape-client/obsolete-results-v
- obsolete-results-v
- Merge into trunk
Revision history for this message
| Muharem Hrnjadovic (al-maisan) wrote : | # |
| 1 | === modified file 'landscape/broker/exchange.py' |
| 2 | --- landscape/broker/exchange.py 2010-04-29 15:57:29 +0000 |
| 3 | +++ landscape/broker/exchange.py 2010-04-30 08:32:27 +0000 |
| 4 | @@ -66,7 +66,7 @@ |
| 5 | return (self._urgent_exchange_interval, self._exchange_interval) |
| 6 | |
| 7 | def _message_is_obsolete(self, message): |
| 8 | - """True if message is obsolete. |
| 9 | + """Returns C{True} if message is obsolete. |
| 10 | |
| 11 | A message is considered obsolete if the secure ID changed since it was |
| 12 | received. |
| 13 | @@ -84,7 +84,10 @@ |
| 14 | |
| 15 | # Compare the current secure ID with the one that was in effect when |
| 16 | # the request message was received. |
| 17 | - return self._registration_info.secure_id != context.secure_id |
| 18 | + result = self._registration_info.secure_id != context.secure_id |
| 19 | + context.remove() |
| 20 | + |
| 21 | + return result |
| 22 | |
| 23 | def send(self, message, urgent=False): |
| 24 | """Include a message to be sent in an exchange. |
| 25 | |
| 26 | === modified file 'landscape/broker/exchangestore.py' |
| 27 | --- landscape/broker/exchangestore.py 2010-04-29 15:57:29 +0000 |
| 28 | +++ landscape/broker/exchangestore.py 2010-04-30 08:35:46 +0000 |
| 29 | @@ -10,7 +10,14 @@ |
| 30 | |
| 31 | |
| 32 | class MessageContext(object): |
| 33 | - """Stores the secure ID for incoming messages that require a response. |
| 34 | + """Stores a context for incoming messages that require a response. |
| 35 | + |
| 36 | + The context consists of |
| 37 | + |
| 38 | + - the "operation-id" value |
| 39 | + - the secure ID that was in effect when the message was received |
| 40 | + - the message type |
| 41 | + - the time when the message was received |
| 42 | |
| 43 | This data will be used to detect secure ID changes between the time at |
| 44 | which the request message came in and the completion of the request. |
| 45 | |
| 46 | === modified file 'landscape/broker/tests/test_exchange.py' |
| 47 | --- landscape/broker/tests/test_exchange.py 2010-04-29 16:05:18 +0000 |
| 48 | +++ landscape/broker/tests/test_exchange.py 2010-04-30 08:29:00 +0000 |
| 49 | @@ -844,6 +844,7 @@ |
| 50 | |
| 51 | # Change the secure ID so that the response message gets discarded. |
| 52 | self.identity.secure_id = 'brand-new' |
| 53 | + ids_before = self.exchanger._store.all_operation_ids() |
| 54 | |
| 55 | self.mstore.set_accepted_types(["resynchronize"]) |
| 56 | message_id = self.exchanger.send( |
| 57 | @@ -858,6 +859,11 @@ |
| 58 | "the client's secure ID has changed in the meantime") |
| 59 | self.assertTrue(expected_log_entry in self.logfile.getvalue()) |
| 60 | |
| 61 | + # The MessageContext was removed after utilisation. |
| 62 | + ids_after = self.exchanger._store.all_operation_ids() |
| 63 | + self.assertTrue(len(ids_after) == len(ids_before) - 1) |
| 64 | + self.assertFalse('234567' in ids_after) |
| 65 | + |
| 66 | |
| 67 | class AcceptedTypesMessageExchangeTest(LandscapeTest): |
| 68 | |
| 69 | |
| 70 | === modified file 'landscape/broker/tests/test_exchangestore.py' |
| 71 | --- landscape/broker/tests/test_exchangestore.py 2010-04-29 15:57:29 +0000 |
| 72 | +++ landscape/broker/tests/test_exchangestore.py 2010-04-30 08:37:03 +0000 |
| 73 | @@ -46,8 +46,9 @@ |
| 74 | self.store1.add_message_context, 123, 'def', 'change-packages') |
| 75 | |
| 76 | def test_get_message_context(self): |
| 77 | - """Accessing a C{MessageContext} with an existing |
| 78 | - C{operation-id} works.""" |
| 79 | + """ |
| 80 | + Accessing a C{MessageContext} with an existing C{operation-id} works. |
| 81 | + """ |
| 82 | now = time.time() |
| 83 | self.store1.add_message_context(234, 'bcd', 'change-packages') |
| 84 | context = self.store2.get_message_context(234) |
| 85 | @@ -69,8 +70,10 @@ |
| 86 | self.assertIs(None, self.store1.get_message_context(345)) |
| 87 | |
| 88 | def test_all_operation_ids_for_empty_database(self): |
| 89 | - """Calling C{all_operation_ids} on an empty database returns an empty |
| 90 | - list.""" |
| 91 | + """ |
| 92 | + Calling C{all_operation_ids} on an empty database returns an empty |
| 93 | + list. |
| 94 | + """ |
| 95 | self.assertEquals([], self.store1.all_operation_ids()) |
| 96 | |
| 97 | def test_all_operation_ids(self): |
On 04/30/2010 09:54 AM, Thomas Herve wrote:
> Review: Needs Fixing
> It looks good!
Hello Thomas,
thank you very much for the review and especially for the first catch below.
I have addressed all of your comments below, please review the changes. An
incremental diff is attached to this message for your convenience.
> [1] As it, the message context store will grow for ever. I think you should remove messages from it as soon as you add it to the message store (or discard it).
>
> [2]
> + def _message_
> + """True if message is obsolete.
>
> Format "Returns C{True} if message is obsolete."
>
> [3]
> +class MessageContext(
> + """Stores the secure ID for incoming messages that require a response.
>
> It seems to store more than that now.
>
> [4]
> + """Accessing a C{MessageContext} with an existing
> + C{operation-id} works."""
>
>
> + """Calling C{all_operation
> + list."""
>
> Please put the docstring on their own lines.
>
>
> Thanks!
Best regards
--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC