)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f280218d48c32cf85c7eb7a8a4991325d4b200b","unresolved":false,"context_lines":[{"line_number":15,"context_line":"Migration.hidden attribute which is not currently used anywhere."},{"line_number":16,"context_line":"If we used that attribute, we could handle duplicate migrations"},{"line_number":17,"context_line":"the same way we handle duplicate instances, but we would need to"},{"line_number":18,"context_line":"think about compatibility impacts to the GET /os-migrations API."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Part of blueprint cross-cell-resize"},{"line_number":21,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"7faddb67_a9d5f544","line":18,"updated":"2019-09-02 14:04:12.000000000","message":"I agree. We can think about such improvement separately.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a8f0bc247e26552ef83f04e138395dd878cb535e","unresolved":false,"context_lines":[{"line_number":12,"context_line":"which picks the record with the newest \"updated_at\" value."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"As noted inline, an alternative to this would be leveraging the"},{"line_number":15,"context_line":"Migration.hidden attribute which is not currently used anywhere."},{"line_number":16,"context_line":"If we used that attribute, we could handle duplicate migrations"},{"line_number":17,"context_line":"the same way we handle duplicate instances, but we would need to"},{"line_number":18,"context_line":"think about compatibility impacts to the GET /os-migrations API."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":50,"id":"3fa7e38b_16f6015f","line":15,"updated":"2019-11-12 15:51:58.000000000","message":"It\u0027s used in the api to filter out those migrations:\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/compute/migrations.py#L56\n\nWe could set hidden\u003dTrue for the older migration, but I\u0027m not sure it would simplify the code any. When looking through the code trying to simplify what you have here, it\u0027s unfortunate to see how many times we iterate the migration list from the lister through to the API. The api uses an obj_to_primitive() but for no reason, which eliminates the ability to return a generator. Moving the filtering logic to the api would remove the extra loop you\u0027re adding, which is tempting, but...","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fcab9cd5a59d442ff495f0ba206e9ea3cc65d8c8","unresolved":false,"context_lines":[{"line_number":12,"context_line":"which picks the record with the newest \"updated_at\" value."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"As noted inline, an alternative to this would be leveraging the"},{"line_number":15,"context_line":"Migration.hidden attribute which is not currently used anywhere."},{"line_number":16,"context_line":"If we used that attribute, we could handle duplicate migrations"},{"line_number":17,"context_line":"the same way we handle duplicate instances, but we would need to"},{"line_number":18,"context_line":"think about compatibility impacts to the GET /os-migrations API."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":50,"id":"3fa7e38b_a981d24f","line":15,"in_reply_to":"3fa7e38b_16f6015f","updated":"2019-11-12 16:14:41.000000000","message":"\u003e It\u0027s used in the api to filter out those migrations:\n \u003e \n \u003e https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/migrations.py#L56\n \u003e \n\nI mean it\u0027s never set to True so that filter is kind of useless today. The API reference also mentions that:\n\nhttps://docs.openstack.org/api-ref/compute/?expanded\u003dlist-migrations-detail#list-migrations\n\n\"But the ‘hidden’ setting of migration is always 0, so this parameter is useless to filter migrations.\"\n\n\u003e The api uses an obj_to_primitive() but for no reason, which eliminates the ability to return a generator.\n\nThat is just dumb code in the route handler and could be fixed separately. What we normally have in a route handler like this is a ViewBuilder implementation which will explicitly pull the fields off the object that go into the response, not what we have here where we convert to dicts and then delete fields which is backward.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b8bef87e00090d3bdca031b863e6bbad762725e8","unresolved":false,"context_lines":[{"line_number":12,"context_line":"which picks the record with the newest \"updated_at\" value."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"As noted inline, an alternative to this would be leveraging the"},{"line_number":15,"context_line":"Migration.hidden attribute which is not currently used anywhere."},{"line_number":16,"context_line":"If we used that attribute, we could handle duplicate migrations"},{"line_number":17,"context_line":"the same way we handle duplicate instances, but we would need to"},{"line_number":18,"context_line":"think about compatibility impacts to the GET /os-migrations API."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":50,"id":"3fa7e38b_5d433412","line":15,"in_reply_to":"3fa7e38b_29cde285","updated":"2019-11-12 20:51:15.000000000","message":"\u003e \u003e I mean it\u0027s never set to True so that filter is kind of useless\n \u003e \u003e today.\n \u003e \n \u003e Sure, but I thought you were saying setting the hidden bit in your\n \u003e loop wouldn\u0027t get honored.\n \u003e \n\nAck, I think I misunderstood your point.\n\nSo we could set migration.hidden\u003dTrue/False through the cross-cell resize operation and just let the API always filter out the hidden\u003dFalse ones like it does today. But unlike the instance being marked hidden or not during the resize and until after the user confirms/reverts it, we copy all of the instance migrations to the target cell DB early:\n\nhttps://github.com/openstack/nova/blob/7aa88029bbf6311033457c32801963da01e88ecb/nova/conductor/tasks/cross_cell_migrate.py#L189\n\nThat\u0027s because once we update the instance mapping to point at the target cell, that\u0027s where the API will go to list migrations for the instance, so we\u0027ll have more duplicates than just the \"active\" migration for the resize operation. To make sure those duplicates are filtered out of the GET /os-migrations API we\u0027d have to also set those to hidden\u003dTrue/False depending on where we are in the operation, i.e. they are hidden in the target DB until the resize is done and we expose the target cell instance.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eca6d204b4aecf649767487b270d88d4e1bf6a73","unresolved":false,"context_lines":[{"line_number":12,"context_line":"which picks the record with the newest \"updated_at\" value."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"As noted inline, an alternative to this would be leveraging the"},{"line_number":15,"context_line":"Migration.hidden attribute which is not currently used anywhere."},{"line_number":16,"context_line":"If we used that attribute, we could handle duplicate migrations"},{"line_number":17,"context_line":"the same way we handle duplicate instances, but we would need to"},{"line_number":18,"context_line":"think about compatibility impacts to the GET /os-migrations API."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":50,"id":"3fa7e38b_29cde285","line":15,"in_reply_to":"3fa7e38b_a981d24f","updated":"2019-11-12 16:24:15.000000000","message":"\u003e I mean it\u0027s never set to True so that filter is kind of useless\n \u003e today.\n\nSure, but I thought you were saying setting the hidden bit in your loop wouldn\u0027t get honored.\n\n \u003e That is just dumb code in the route handler and could be fixed\n \u003e separately. What we normally have in a route handler like this is a\n \u003e ViewBuilder implementation which will explicitly pull the fields\n \u003e off the object that go into the response, not what we have here\n \u003e where we convert to dicts and then delete fields which is backward.\n\nYeah, so if we were to fix that we could make your new loop return a generator to do the filtering inline and collapse a lot of the repetition. However, I know, it\u0027s not a big deal, this stuff just builds up over time to get us a super inefficient overall system.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"}],"nova/compute/api.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f280218d48c32cf85c7eb7a8a4991325d4b200b","unresolved":false,"context_lines":[{"line_number":4693,"context_line":"                migrations_by_uuid[migration.uuid] \u003d migration"},{"line_number":4694,"context_line":"            else:"},{"line_number":4695,"context_line":"                # We have a collision, keep the one with the later updated_at."},{"line_number":4696,"context_line":"                # FIXME(mriedem): Using updated_at could be wrong if"},{"line_number":4697,"context_line":"                # changes-since or changes-before filters are being used."},{"line_number":4698,"context_line":"                doppleganger \u003d migrations_by_uuid[migration.uuid]"},{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"}],"source_content_type":"text/x-python","patch_set":38,"id":"7faddb67_e9b68d25","line":4697,"range":{"start_line":4696,"start_character":1,"end_line":4697,"end_character":73},"updated":"2019-09-02 14:04:12.000000000","message":"migration.hidden would not solve this either as we don\u0027t know which migration was active at which time window. So I guess we cannot really support changes-before filters here.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4079dbe59fa034be6eff6df0d7932389f8aff939","unresolved":false,"context_lines":[{"line_number":4693,"context_line":"                migrations_by_uuid[migration.uuid] \u003d migration"},{"line_number":4694,"context_line":"            else:"},{"line_number":4695,"context_line":"                # We have a collision, keep the one with the later updated_at."},{"line_number":4696,"context_line":"                # FIXME(mriedem): Using updated_at could be wrong if"},{"line_number":4697,"context_line":"                # changes-since or changes-before filters are being used."},{"line_number":4698,"context_line":"                doppleganger \u003d migrations_by_uuid[migration.uuid]"},{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"}],"source_content_type":"text/x-python","patch_set":38,"id":"3fa7e38b_fa1fa358","line":4697,"range":{"start_line":4696,"start_character":1,"end_line":4697,"end_character":73},"in_reply_to":"7faddb67_e9b68d25","updated":"2019-10-21 18:42:54.000000000","message":"Maybe I should just remove this FIXME comment. _get_unique_filter_method has the same issue.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f280218d48c32cf85c7eb7a8a4991325d4b200b","unresolved":false,"context_lines":[{"line_number":4696,"context_line":"                # FIXME(mriedem): Using updated_at could be wrong if"},{"line_number":4697,"context_line":"                # changes-since or changes-before filters are being used."},{"line_number":4698,"context_line":"                doppleganger \u003d migrations_by_uuid[migration.uuid]"},{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"},{"line_number":4701,"context_line":"                if old_updated_at is None and new_updated_at is not None:"},{"line_number":4702,"context_line":"                    # Replace the entry in the dict but maintain the order."}],"source_content_type":"text/x-python","patch_set":38,"id":"7faddb67_690e1de5","line":4699,"range":{"start_line":4699,"start_character":16,"end_line":4699,"end_character":30},"updated":"2019-09-02 14:04:12.000000000","message":"old and new is double loaded here as it talks about time. first_updated_at , second_updated_at feels bad too. Maybe already_found_updated_at, newly_found_update_at would be better but that is long.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4079dbe59fa034be6eff6df0d7932389f8aff939","unresolved":false,"context_lines":[{"line_number":4696,"context_line":"                # FIXME(mriedem): Using updated_at could be wrong if"},{"line_number":4697,"context_line":"                # changes-since or changes-before filters are being used."},{"line_number":4698,"context_line":"                doppleganger \u003d migrations_by_uuid[migration.uuid]"},{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"},{"line_number":4701,"context_line":"                if old_updated_at is None and new_updated_at is not None:"},{"line_number":4702,"context_line":"                    # Replace the entry in the dict but maintain the order."}],"source_content_type":"text/x-python","patch_set":38,"id":"3fa7e38b_7a48935b","line":4699,"range":{"start_line":4699,"start_character":16,"end_line":4699,"end_character":30},"in_reply_to":"7faddb67_690e1de5","updated":"2019-10-21 18:42:54.000000000","message":"How about mapped_updated_at and incoming_updated_at or something like that?","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f280218d48c32cf85c7eb7a8a4991325d4b200b","unresolved":false,"context_lines":[{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"},{"line_number":4701,"context_line":"                if old_updated_at is None and new_updated_at is not None:"},{"line_number":4702,"context_line":"                    # Replace the entry in the dict but maintain the order."},{"line_number":4703,"context_line":"                    migrations_by_uuid[migration.uuid] \u003d migration"},{"line_number":4704,"context_line":"                elif (new_updated_at is not None and"},{"line_number":4705,"context_line":"                        new_updated_at \u003e old_updated_at):"}],"source_content_type":"text/x-python","patch_set":38,"id":"7faddb67_09e5290a","line":4702,"updated":"2019-09-02 14:04:12.000000000","message":"Don\u0027t we have to compare doppleganger.created_at with migration.update_at here?","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b6c1965f50144566fe6e64690fa9082bacca5fc1","unresolved":false,"context_lines":[{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"},{"line_number":4701,"context_line":"                if old_updated_at is None and new_updated_at is not None:"},{"line_number":4702,"context_line":"                    # Replace the entry in the dict but maintain the order."},{"line_number":4703,"context_line":"                    migrations_by_uuid[migration.uuid] \u003d migration"},{"line_number":4704,"context_line":"                elif (new_updated_at is not None and"},{"line_number":4705,"context_line":"                        new_updated_at \u003e old_updated_at):"}],"source_content_type":"text/x-python","patch_set":38,"id":"3fa7e38b_7a6bb384","line":4702,"in_reply_to":"3fa7e38b_5a9ef7b8","updated":"2019-10-21 18:57:30.000000000","message":"I suppose you could also have a case where doppleganger.updated_at is not None but migration.updated_at is None and migration.created_at \u003e doppleganger.updated_at.\n\nI should probably re-write this logic into a simple function to just return the newer of two objects and always update the dict.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4079dbe59fa034be6eff6df0d7932389f8aff939","unresolved":false,"context_lines":[{"line_number":4699,"context_line":"                old_updated_at \u003d doppleganger.updated_at"},{"line_number":4700,"context_line":"                new_updated_at \u003d migration.updated_at"},{"line_number":4701,"context_line":"                if old_updated_at is None and new_updated_at is not None:"},{"line_number":4702,"context_line":"                    # Replace the entry in the dict but maintain the order."},{"line_number":4703,"context_line":"                    migrations_by_uuid[migration.uuid] \u003d migration"},{"line_number":4704,"context_line":"                elif (new_updated_at is not None and"},{"line_number":4705,"context_line":"                        new_updated_at \u003e old_updated_at):"}],"source_content_type":"text/x-python","patch_set":38,"id":"3fa7e38b_5a9ef7b8","line":4702,"in_reply_to":"7faddb67_09e5290a","updated":"2019-10-21 18:42:54.000000000","message":"Meaning if doppleganger.updated_at is None but doppleganger.created_at is \u003e migration.updated_at, we don\u0027t overwrite doppleganger in the dict because it\u0027s technically newer, right? I\u0027ll add a test and condition for it.","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_c792f5fc","line":4954,"updated":"2019-11-12 18:44:47.000000000","message":"If you don\u0027t want to use ddt and just implement the suggested reversed, this is the only case you\u0027re missing.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_a2d1e7ce","line":4954,"in_reply_to":"3fa7e38b_c792f5fc","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a8f0bc247e26552ef83f04e138395dd878cb535e","unresolved":false,"context_lines":[{"line_number":4936,"context_line":"            # updated_at might be None"},{"line_number":4937,"context_line":"            updated_at1 \u003d obj1.updated_at"},{"line_number":4938,"context_line":"            updated_at2 \u003d obj2.updated_at"},{"line_number":4939,"context_line":"            # If both have updated_at, compare using that field."},{"line_number":4940,"context_line":"            if updated_at1 and updated_at2:"},{"line_number":4941,"context_line":"                if updated_at1 \u003e updated_at2:"},{"line_number":4942,"context_line":"                    return obj1"},{"line_number":4943,"context_line":"                return obj2"},{"line_number":4944,"context_line":"            # Compare created_at versus updated_at."},{"line_number":4945,"context_line":"            if updated_at1:"},{"line_number":4946,"context_line":"                if updated_at1 \u003e created_at2:"},{"line_number":4947,"context_line":"                    return obj1"},{"line_number":4948,"context_line":"                return obj2"},{"line_number":4949,"context_line":"            if updated_at2:"},{"line_number":4950,"context_line":"                if updated_at2 \u003e created_at1:"},{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""},{"line_number":4958,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4959,"context_line":"        # field on the Migration record and then just did like"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_f6f00594","line":4956,"range":{"start_line":4939,"start_character":0,"end_line":4956,"end_character":23},"updated":"2019-11-12 15:51:58.000000000","message":"Can\u0027t you replace all of this with:\n\n if (updated_at1, created_at1) \u003c (updated_at2, created_at2):\n     return obj1\n else:\n     return obj2\n\n?","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":4937,"context_line":"            updated_at1 \u003d obj1.updated_at"},{"line_number":4938,"context_line":"            updated_at2 \u003d obj2.updated_at"},{"line_number":4939,"context_line":"            # If both have updated_at, compare using that field."},{"line_number":4940,"context_line":"            if updated_at1 and updated_at2:"},{"line_number":4941,"context_line":"                if updated_at1 \u003e updated_at2:"},{"line_number":4942,"context_line":"                    return obj1"},{"line_number":4943,"context_line":"                return obj2"},{"line_number":4944,"context_line":"            # Compare created_at versus updated_at."},{"line_number":4945,"context_line":"            if updated_at1:"},{"line_number":4946,"context_line":"                if updated_at1 \u003e created_at2:"},{"line_number":4947,"context_line":"                    return obj1"},{"line_number":4948,"context_line":"                return obj2"},{"line_number":4949,"context_line":"            if updated_at2:"},{"line_number":4950,"context_line":"                if updated_at2 \u003e created_at1:"},{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""},{"line_number":4958,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4959,"context_line":"        # field on the Migration record and then just did like"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_473cc52f","line":4956,"range":{"start_line":4940,"start_character":0,"end_line":4956,"end_character":23},"updated":"2019-11-12 18:44:47.000000000","message":"You haven\u0027t got complete unit test coverage here. Suggest ddt would be a simple way to go.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fcab9cd5a59d442ff495f0ba206e9ea3cc65d8c8","unresolved":false,"context_lines":[{"line_number":4936,"context_line":"            # updated_at might be None"},{"line_number":4937,"context_line":"            updated_at1 \u003d obj1.updated_at"},{"line_number":4938,"context_line":"            updated_at2 \u003d obj2.updated_at"},{"line_number":4939,"context_line":"            # If both have updated_at, compare using that field."},{"line_number":4940,"context_line":"            if updated_at1 and updated_at2:"},{"line_number":4941,"context_line":"                if updated_at1 \u003e updated_at2:"},{"line_number":4942,"context_line":"                    return obj1"},{"line_number":4943,"context_line":"                return obj2"},{"line_number":4944,"context_line":"            # Compare created_at versus updated_at."},{"line_number":4945,"context_line":"            if updated_at1:"},{"line_number":4946,"context_line":"                if updated_at1 \u003e created_at2:"},{"line_number":4947,"context_line":"                    return obj1"},{"line_number":4948,"context_line":"                return obj2"},{"line_number":4949,"context_line":"            if updated_at2:"},{"line_number":4950,"context_line":"                if updated_at2 \u003e created_at1:"},{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""},{"line_number":4958,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4959,"context_line":"        # field on the Migration record and then just did like"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_e9e34a33","line":4956,"range":{"start_line":4939,"start_character":0,"end_line":4956,"end_character":23},"in_reply_to":"3fa7e38b_7643b56d","updated":"2019-11-12 16:14:41.000000000","message":"https://pastebin.com/pHtM7iEf","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":4936,"context_line":"            # updated_at might be None"},{"line_number":4937,"context_line":"            updated_at1 \u003d obj1.updated_at"},{"line_number":4938,"context_line":"            updated_at2 \u003d obj2.updated_at"},{"line_number":4939,"context_line":"            # If both have updated_at, compare using that field."},{"line_number":4940,"context_line":"            if updated_at1 and updated_at2:"},{"line_number":4941,"context_line":"                if updated_at1 \u003e updated_at2:"},{"line_number":4942,"context_line":"                    return obj1"},{"line_number":4943,"context_line":"                return obj2"},{"line_number":4944,"context_line":"            # Compare created_at versus updated_at."},{"line_number":4945,"context_line":"            if updated_at1:"},{"line_number":4946,"context_line":"                if updated_at1 \u003e created_at2:"},{"line_number":4947,"context_line":"                    return obj1"},{"line_number":4948,"context_line":"                return obj2"},{"line_number":4949,"context_line":"            if updated_at2:"},{"line_number":4950,"context_line":"                if updated_at2 \u003e created_at1:"},{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""},{"line_number":4958,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4959,"context_line":"        # field on the Migration record and then just did like"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_027ebbae","line":4956,"range":{"start_line":4939,"start_character":0,"end_line":4956,"end_character":23},"in_reply_to":"3fa7e38b_f6f00594","updated":"2019-11-12 20:22:52.000000000","message":"\u003e Can\u0027t you replace all of this with:\n \u003e \n \u003e if (updated_at1, created_at1) \u003c (updated_at2, created_at2):\n \u003e return obj1\n \u003e else:\n \u003e return obj2\n \u003e \n \u003e ?\n\nAs discussed, this doesn\u0027t handle updated_at being None, but you fixed that in your paste below.\n\nAnd the logic above is wrong since the function is supposed to return the newer object so the \u003c should be \u003e or swap the objects being returned. That\u0027s an easy fix.\n\nBut the bigger issue is test_get_migrations_sorted_filter_duplicates_using_created_at fails with this and it\u0027s not the same as the logic I have, which says that if an object has a newer created_at than the other object\u0027s updated_at, we take the object with the newer created_at. Your paste with handling None is not handling that properly.\n\nUnless there is a simple way to make that work with the tuples, I think it\u0027s likely not worth the complexity/readability since what I have here is explicit.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0258bd8a44d48b1278772e836f00a44ed8b86422","unresolved":false,"context_lines":[{"line_number":4936,"context_line":"            # updated_at might be None"},{"line_number":4937,"context_line":"            updated_at1 \u003d obj1.updated_at"},{"line_number":4938,"context_line":"            updated_at2 \u003d obj2.updated_at"},{"line_number":4939,"context_line":"            # If both have updated_at, compare using that field."},{"line_number":4940,"context_line":"            if updated_at1 and updated_at2:"},{"line_number":4941,"context_line":"                if updated_at1 \u003e updated_at2:"},{"line_number":4942,"context_line":"                    return obj1"},{"line_number":4943,"context_line":"                return obj2"},{"line_number":4944,"context_line":"            # Compare created_at versus updated_at."},{"line_number":4945,"context_line":"            if updated_at1:"},{"line_number":4946,"context_line":"                if updated_at1 \u003e created_at2:"},{"line_number":4947,"context_line":"                    return obj1"},{"line_number":4948,"context_line":"                return obj2"},{"line_number":4949,"context_line":"            if updated_at2:"},{"line_number":4950,"context_line":"                if updated_at2 \u003e created_at1:"},{"line_number":4951,"context_line":"                    return obj2"},{"line_number":4952,"context_line":"                return obj1"},{"line_number":4953,"context_line":"            # Compare created_at only."},{"line_number":4954,"context_line":"            if created_at1 \u003e created_at2:"},{"line_number":4955,"context_line":"                return obj1"},{"line_number":4956,"context_line":"            return obj2"},{"line_number":4957,"context_line":""},{"line_number":4958,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4959,"context_line":"        # field on the Migration record and then just did like"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_7643b56d","line":4956,"range":{"start_line":4939,"start_character":0,"end_line":4956,"end_character":23},"in_reply_to":"3fa7e38b_f6f00594","updated":"2019-11-12 15:59:20.000000000","message":"Per the comment, updated_at might be None and you can\u0027t compare datetime objects to None:\n\n\u003e\u003e\u003e now1 \u003d datetime.datetime.now()\n\u003e\u003e\u003e now2 \u003d datetime.datetime.now()\n\u003e\u003e\u003e now1 \u003c now2\nTrue\n\u003e\u003e\u003e now1 \u003c None\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\nTypeError: can\u0027t compare datetime.datetime to NoneType","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"652deef58cc1ae0d7ca6bd4ced198a47f48283aa","unresolved":false,"context_lines":[{"line_number":4953,"context_line":"                return obj1"},{"line_number":4954,"context_line":"            return obj2"},{"line_number":4955,"context_line":""},{"line_number":4956,"context_line":"        # TODO(mriedem): This could be easier if we leveraged the \"hidden\""},{"line_number":4957,"context_line":"        # field on the Migration record and then just did like"},{"line_number":4958,"context_line":"        # _get_unique_filter_method in the get_all() method for instances."},{"line_number":4959,"context_line":"        migrations_by_uuid \u003d collections.OrderedDict()  # maintain sort order"}],"source_content_type":"text/x-python","patch_set":52,"id":"3fa7e38b_6815ed3c","line":4956,"updated":"2019-11-14 17:05:01.000000000","message":"As noted earlier in discussion, during a cross cell migration we copy the existing migration records from the source cell to the target cell db so we\u0027d maybe have to mark more than just the \u0027current\u0027 migration as hidden, i.e. all of the existing ones too while they are in the target cell db before we flip the instance mapping to point at the target cell. Doing that would be kind of nasty.\n\nThe one saving thing there would be any existing completed migrations that get copied are likely really just duplicates, so their created_at/updated_at values aren\u0027t going to change since they are old and done, so for those we could likely just do a simple duplication filter by uuid (like _get_unique_filter_method for instances). If that worked, then it would really be only the \u0027active\u0027 migration that needs the hidden flag management and we can filter that thing out in the DB API like we do for the hidden instance.","commit_id":"63094c288317b7624e43f4f0d9c712d6576b663d"}],"nova/tests/unit/compute/test_compute_api.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5f280218d48c32cf85c7eb7a8a4991325d4b200b","unresolved":false,"context_lines":[{"line_number":6679,"context_line":"        target_cell_migration \u003d objects.Migration("},{"line_number":6680,"context_line":"            uuid\u003duuids.migration, updated_at\u003dtimeutils.utcnow())"},{"line_number":6681,"context_line":"        # Sanity check our updated_at values."},{"line_number":6682,"context_line":"        self.assertGreater(target_cell_migration.updated_at,"},{"line_number":6683,"context_line":"                           source_cell_migration.updated_at)"},{"line_number":6684,"context_line":"        with mock.patch("},{"line_number":6685,"context_line":"                \u0027nova.compute.migration_list.get_migration_objects_sorted\u0027,"},{"line_number":6686,"context_line":"                return_value\u003dobjects.MigrationList(objects\u003d["}],"source_content_type":"text/x-python","patch_set":38,"id":"7faddb67_89ba79de","line":6683,"range":{"start_line":6682,"start_character":0,"end_line":6683,"end_character":60},"updated":"2019-09-02 14:04:12.000000000","message":"better safe than sorry :)","commit_id":"7e0adc2905d4c546ce54b1d7dada11ddf2b09a88"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":6688,"context_line":"                \u0027nova.compute.migration_list.get_migration_objects_sorted\u0027,"},{"line_number":6689,"context_line":"                return_value\u003dobjects.MigrationList("},{"line_number":6690,"context_line":"                    objects\u003dmigrations)) as getter:"},{"line_number":6691,"context_line":"            migrations \u003d self.compute_api.get_migrations_sorted("},{"line_number":6692,"context_line":"                self.context, filters, sort_dirs\u003dsort_dirs,"},{"line_number":6693,"context_line":"                sort_keys\u003dsort_keys, limit\u003dlimit, marker\u003dmarker)"},{"line_number":6694,"context_line":"        self.assertEqual(1, len(migrations))"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_e7a131aa","line":6691,"range":{"start_line":6691,"start_character":12,"end_line":6691,"end_character":22},"updated":"2019-11-12 18:44:47.000000000","message":"nit: this aliases an argument. Would be more readable if it didn\u0027t.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":6688,"context_line":"                \u0027nova.compute.migration_list.get_migration_objects_sorted\u0027,"},{"line_number":6689,"context_line":"                return_value\u003dobjects.MigrationList("},{"line_number":6690,"context_line":"                    objects\u003dmigrations)) as getter:"},{"line_number":6691,"context_line":"            migrations \u003d self.compute_api.get_migrations_sorted("},{"line_number":6692,"context_line":"                self.context, filters, sort_dirs\u003dsort_dirs,"},{"line_number":6693,"context_line":"                sort_keys\u003dsort_keys, limit\u003dlimit, marker\u003dmarker)"},{"line_number":6694,"context_line":"        self.assertEqual(1, len(migrations))"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_a20ec789","line":6691,"range":{"start_line":6691,"start_character":12,"end_line":6691,"end_character":22},"in_reply_to":"3fa7e38b_e7a131aa","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":6703,"context_line":"        t1 \u003d timeutils.utcnow()"},{"line_number":6704,"context_line":"        source_cell_migration \u003d objects.Migration("},{"line_number":6705,"context_line":"            uuid\u003duuids.migration, created_at\u003dt1, updated_at\u003dt1)"},{"line_number":6706,"context_line":"        t2 \u003d timeutils.utcnow()"},{"line_number":6707,"context_line":"        target_cell_migration \u003d objects.Migration("},{"line_number":6708,"context_line":"            uuid\u003duuids.migration, created_at\u003dt2, updated_at\u003dt2)"},{"line_number":6709,"context_line":"        # Sanity check our updated_at values."}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_a7001968","line":6706,"updated":"2019-11-12 18:44:47.000000000","message":"Suggestion: This is probably always going to work unless our CI infrastructure gets really, really fast, or the implementation of utcnow() loses granularity. However, it would be guaranteed to work and a bit clearer to do:\n\nt2 \u003d t1 + datetime.timedelta(seconds\u003d1)\n\nYou could also remove the sanity check.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":6703,"context_line":"        t1 \u003d timeutils.utcnow()"},{"line_number":6704,"context_line":"        source_cell_migration \u003d objects.Migration("},{"line_number":6705,"context_line":"            uuid\u003duuids.migration, created_at\u003dt1, updated_at\u003dt1)"},{"line_number":6706,"context_line":"        t2 \u003d timeutils.utcnow()"},{"line_number":6707,"context_line":"        target_cell_migration \u003d objects.Migration("},{"line_number":6708,"context_line":"            uuid\u003duuids.migration, created_at\u003dt2, updated_at\u003dt2)"},{"line_number":6709,"context_line":"        # Sanity check our updated_at values."}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_021f9bb7","line":6706,"in_reply_to":"3fa7e38b_a7001968","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":6711,"context_line":"                           source_cell_migration.updated_at)"},{"line_number":6712,"context_line":"        self._test_get_migrations_sorted_filter_duplicates("},{"line_number":6713,"context_line":"            [source_cell_migration, target_cell_migration],"},{"line_number":6714,"context_line":"            target_cell_migration)"},{"line_number":6715,"context_line":""},{"line_number":6716,"context_line":"    def test_get_migrations_sorted_filter_duplicates_using_created_at(self):"},{"line_number":6717,"context_line":"        \"\"\"Tests the cross-cell scenario where there are multiple migrations"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_a7ea5971","line":6714,"updated":"2019-11-12 18:44:47.000000000","message":"Suggestion: if you don\u0027t want to use ddt, you can double your test coverage by executing this again with source and target reversed.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":6711,"context_line":"                           source_cell_migration.updated_at)"},{"line_number":6712,"context_line":"        self._test_get_migrations_sorted_filter_duplicates("},{"line_number":6713,"context_line":"            [source_cell_migration, target_cell_migration],"},{"line_number":6714,"context_line":"            target_cell_migration)"},{"line_number":6715,"context_line":""},{"line_number":6716,"context_line":"    def test_get_migrations_sorted_filter_duplicates_using_created_at(self):"},{"line_number":6717,"context_line":"        \"\"\"Tests the cross-cell scenario where there are multiple migrations"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_426a731b","line":6714,"in_reply_to":"3fa7e38b_a7ea5971","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":6722,"context_line":"        t1 \u003d timeutils.utcnow()"},{"line_number":6723,"context_line":"        older \u003d objects.Migration("},{"line_number":6724,"context_line":"            uuid\u003duuids.migration, created_at\u003dt1, updated_at\u003dt1)"},{"line_number":6725,"context_line":"        t2 \u003d timeutils.utcnow()"},{"line_number":6726,"context_line":"        newer \u003d objects.Migration("},{"line_number":6727,"context_line":"            uuid\u003duuids.migration, created_at\u003dt2, updated_at\u003dNone)"},{"line_number":6728,"context_line":"        # Sanity check our values."}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_6737814d","line":6725,"updated":"2019-11-12 18:44:47.000000000","message":"Ditto.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":6722,"context_line":"        t1 \u003d timeutils.utcnow()"},{"line_number":6723,"context_line":"        older \u003d objects.Migration("},{"line_number":6724,"context_line":"            uuid\u003duuids.migration, created_at\u003dt1, updated_at\u003dt1)"},{"line_number":6725,"context_line":"        t2 \u003d timeutils.utcnow()"},{"line_number":6726,"context_line":"        newer \u003d objects.Migration("},{"line_number":6727,"context_line":"            uuid\u003duuids.migration, created_at\u003dt2, updated_at\u003dNone)"},{"line_number":6728,"context_line":"        # Sanity check our values."}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_62652f49","line":6725,"in_reply_to":"3fa7e38b_6737814d","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e778fed6b6b5cfa8e47aa00ffb70ccc89350c47e","unresolved":false,"context_lines":[{"line_number":6732,"context_line":"        # Test with just created_at."},{"line_number":6733,"context_line":"        older.updated_at \u003d None"},{"line_number":6734,"context_line":"        self._test_get_migrations_sorted_filter_duplicates("},{"line_number":6735,"context_line":"            [newer, older], newer)"},{"line_number":6736,"context_line":""},{"line_number":6737,"context_line":""},{"line_number":6738,"context_line":"class DiffDictTestCase(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_47012539","line":6735,"updated":"2019-11-12 18:44:47.000000000","message":"Ditto.","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dc6342d01bf5af59636f079873d2a961b70ad7a3","unresolved":false,"context_lines":[{"line_number":6732,"context_line":"        # Test with just created_at."},{"line_number":6733,"context_line":"        older.updated_at \u003d None"},{"line_number":6734,"context_line":"        self._test_get_migrations_sorted_filter_duplicates("},{"line_number":6735,"context_line":"            [newer, older], newer)"},{"line_number":6736,"context_line":""},{"line_number":6737,"context_line":""},{"line_number":6738,"context_line":"class DiffDictTestCase(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":50,"id":"3fa7e38b_02747bfd","line":6735,"in_reply_to":"3fa7e38b_47012539","updated":"2019-11-12 20:22:52.000000000","message":"Done","commit_id":"703a7bf3d8e5b59824689871d3851d5b304194a4"}]}