)]}'
{"trove/guestagent/datastore/postgresql/manager.py":[{"author":{"_account_id":13355,"name":"Edmond Kotowski","email":"ekotowski@gmail.com","username":"ekotowski"},"change_message_id":"1ceb667a63b732369713d867798ba361577dfdaa","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            device \u003d volume.VolumeDevice(device_path)"},{"line_number":72,"context_line":"            device.format()"},{"line_number":73,"context_line":"            if os.path.exists(mount_point):"},{"line_number":74,"context_line":"                device.migrate_data(mount_point)"},{"line_number":75,"context_line":"            device.mount(mount_point)"},{"line_number":76,"context_line":"        self.reset_configuration(context, config_contents)"},{"line_number":77,"context_line":"        self.set_db_to_listen(context)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a961159_0ee2ddd4","line":74,"updated":"2015-01-14 20:43:54.000000000","message":"Looking at taskmanager/models.py Line 272\n\nbackup_info \u003d None\nif backup_id is not None:\n        backup \u003d bkup_models.Backup.get_by_id(self.context, \n        \t\t\t\t\t\t\t\t      backup_id)\n        backup_info \u003d {\u0027id\u0027: backup_id,\n                       \u0027location\u0027: backup.location,\n                       \u0027type\u0027: backup.backup_type,\n                       \u0027checksum\u0027: backup.checksum,\n                       }\n\nself._guest_prepare(flavor[\u0027ram\u0027], volume_info,\n                            packages, databases, users, backup_info,\n                            config.config_contents, root_password,\n                            config_overrides.config_contents,\n                            cluster_config)\n\nIt seems like we not passing in the backup_id into create_instance which causes backup_info to be None. Is backup_info really not needed, because it seems like we would want this information when doing a backup especially for the backup.restore(context, backup_info, \u0027/tmp\u0027) call?","commit_id":"4416577cc11e00a3e52dc50490be75f4931c7ad8"},{"author":{"_account_id":7850,"name":"Kevin Conway","email":"kevinjacobconway@gmail.com","username":"kevinconway"},"change_message_id":"4919950037055bc8b68c72a40194d077054d281a","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            device \u003d volume.VolumeDevice(device_path)"},{"line_number":72,"context_line":"            device.format()"},{"line_number":73,"context_line":"            if os.path.exists(mount_point):"},{"line_number":74,"context_line":"                device.migrate_data(mount_point)"},{"line_number":75,"context_line":"            device.mount(mount_point)"},{"line_number":76,"context_line":"        self.reset_configuration(context, config_contents)"},{"line_number":77,"context_line":"        self.set_db_to_listen(context)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_082ffff6","line":74,"updated":"2015-01-16 00:14:40.000000000","message":"The issue you described was the migration not happening when a backup_info object was passed in. Wouldn\u0027t removing this check cause the migration to happen _every_ time?","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":14576,"name":"Petr Malik","email":"pmalik@tesora.com","username":"pmalik"},"change_message_id":"a259ff18144bd09db8c2576d261a6cc164891ef3","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            device \u003d volume.VolumeDevice(device_path)"},{"line_number":72,"context_line":"            device.format()"},{"line_number":73,"context_line":"            if os.path.exists(mount_point):"},{"line_number":74,"context_line":"                device.migrate_data(mount_point)"},{"line_number":75,"context_line":"            device.mount(mount_point)"},{"line_number":76,"context_line":"        self.reset_configuration(context, config_contents)"},{"line_number":77,"context_line":"        self.set_db_to_listen(context)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_c1ddbb15","line":74,"in_reply_to":"3a961159_082ffff6","updated":"2015-01-16 16:40:16.000000000","message":"Yes. This is exactly what it is supposed to do. Our testing suggests that Postgres wouldn\u0027t start without having the data directory migrated at the first place. Do you know about any case when it should not happen? Thanks.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":7850,"name":"Kevin Conway","email":"kevinjacobconway@gmail.com","username":"kevinconway"},"change_message_id":"16d0c96088b436cfc0dde945a302f0763017fefd","unresolved":false,"context_lines":[{"line_number":71,"context_line":"            device \u003d volume.VolumeDevice(device_path)"},{"line_number":72,"context_line":"            device.format()"},{"line_number":73,"context_line":"            if os.path.exists(mount_point):"},{"line_number":74,"context_line":"                device.migrate_data(mount_point)"},{"line_number":75,"context_line":"            device.mount(mount_point)"},{"line_number":76,"context_line":"        self.reset_configuration(context, config_contents)"},{"line_number":77,"context_line":"        self.set_db_to_listen(context)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_73120bb5","line":74,"in_reply_to":"3a961159_c1ddbb15","updated":"2015-01-17 03:01:54.000000000","message":"No, I think I understand what you\u0027re getting at. It should always copy over the PostgreSQL files regardless of whether or not there is a backup.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"}],"trove/guestagent/strategies/restore/postgresql_impl.py":[{"author":{"_account_id":7850,"name":"Kevin Conway","email":"kevinjacobconway@gmail.com","username":"kevinconway"},"change_message_id":"4919950037055bc8b68c72a40194d077054d281a","unresolved":false,"context_lines":[{"line_number":35,"context_line":"            return content_length"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def _execute_postgres_restore(self):"},{"line_number":38,"context_line":"        # We are overriding the base class behavior"},{"line_number":39,"context_line":"        # to perform custom error handling."},{"line_number":40,"context_line":"        # Postgresql outputs few benign messages into the stderr stream"},{"line_number":41,"context_line":"        # during a normal restore procedure."}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_884f6fbd","line":38,"updated":"2015-01-16 00:14:40.000000000","message":"I think documenting why you are implementing this custom behaviour is great. You should turn these comments into an actual docstring so they are available through all the normal Python mechanisms.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":14576,"name":"Petr Malik","email":"pmalik@tesora.com","username":"pmalik"},"change_message_id":"a259ff18144bd09db8c2576d261a6cc164891ef3","unresolved":false,"context_lines":[{"line_number":35,"context_line":"            return content_length"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    def _execute_postgres_restore(self):"},{"line_number":38,"context_line":"        # We are overriding the base class behavior"},{"line_number":39,"context_line":"        # to perform custom error handling."},{"line_number":40,"context_line":"        # Postgresql outputs few benign messages into the stderr stream"},{"line_number":41,"context_line":"        # during a normal restore procedure."}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_e198f71c","line":38,"in_reply_to":"3a961159_884f6fbd","updated":"2015-01-16 16:40:16.000000000","message":"OK, I generally agree with this. The first line should really be a documentation comment on the restore function. I would leave the rest as is since it describes implementation details that generally do not belong to the documentation.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"a9273b2e5126d4a287b6ac0f7933ada6f1b0685d","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        # is expected and does not pose any problems to the restore operation."},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        stream \u003d self.storage.load(self.location, self.checksum)"},{"line_number":48,"context_line":"        process \u003d subprocess.Popen(self.restore_cmd, shell\u003dTrue,"},{"line_number":49,"context_line":"                                   stdin\u003dsubprocess.PIPE,"},{"line_number":50,"context_line":"                                   stderr\u003dsubprocess.PIPE)"},{"line_number":51,"context_line":"        content_length \u003d 0"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_e45d5349","line":48,"updated":"2015-01-17 12:24:57.000000000","message":"please don\u0027t do this ... use oslo.concurrency.execute()","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":9664,"name":"amrith","email":"amrith.kumar@gmail.com","username":"amrith"},"change_message_id":"96c61e98d5cba619465f8eefefa405ee1f2428d4","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        # is expected and does not pose any problems to the restore operation."},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        stream \u003d self.storage.load(self.location, self.checksum)"},{"line_number":48,"context_line":"        process \u003d subprocess.Popen(self.restore_cmd, shell\u003dTrue,"},{"line_number":49,"context_line":"                                   stdin\u003dsubprocess.PIPE,"},{"line_number":50,"context_line":"                                   stderr\u003dsubprocess.PIPE)"},{"line_number":51,"context_line":"        content_length \u003d 0"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_dd0bac20","line":48,"in_reply_to":"3a961159_6764e921","updated":"2015-01-19 16:39:15.000000000","message":"sure thing!","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":14576,"name":"Petr Malik","email":"pmalik@tesora.com","username":"pmalik"},"change_message_id":"2618c4aada95ef339a39d0e0ab4a8f6d4f7428f9","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        # is expected and does not pose any problems to the restore operation."},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        stream \u003d self.storage.load(self.location, self.checksum)"},{"line_number":48,"context_line":"        process \u003d subprocess.Popen(self.restore_cmd, shell\u003dTrue,"},{"line_number":49,"context_line":"                                   stdin\u003dsubprocess.PIPE,"},{"line_number":50,"context_line":"                                   stderr\u003dsubprocess.PIPE)"},{"line_number":51,"context_line":"        content_length \u003d 0"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_6764e921","line":48,"in_reply_to":"3a961159_e45d5349","updated":"2015-01-19 16:05:10.000000000","message":"I did it this way to keep it consistent with the base class (the code snipped was actually taken from the base class).\nWould it be acceptable if we kept the old call for this patch set and filed a new bug to update both base and derived classes at once?\n\nThanks.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":7850,"name":"Kevin Conway","email":"kevinjacobconway@gmail.com","username":"kevinconway"},"change_message_id":"4919950037055bc8b68c72a40194d077054d281a","unresolved":false,"context_lines":[{"line_number":69,"context_line":"            # for future compatibility."},{"line_number":70,"context_line":"            if err:"},{"line_number":71,"context_line":"                err_messages \u003d err.splitlines(False)"},{"line_number":72,"context_line":"                if err_messages !\u003d self.IGNORED_ERROR_MSG_SEQUENCE:"},{"line_number":73,"context_line":"                    raise exception(err)"},{"line_number":74,"context_line":"        except OSError:"},{"line_number":75,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_88684f22","line":72,"updated":"2015-01-16 00:14:40.000000000","message":"You should consider iterating over the error messages and looking them up in the ignored messages list using \u0027in\u0027, or something similar. That would give you an opportunity to add additional ignored error messages in the future without refactoring this code.\n\nExample:\n\nfor message in err_messages:\n    if message not in self.IGNORED_MESSAGES:\n        raise exception(err)","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":7850,"name":"Kevin Conway","email":"kevinjacobconway@gmail.com","username":"kevinconway"},"change_message_id":"16d0c96088b436cfc0dde945a302f0763017fefd","unresolved":false,"context_lines":[{"line_number":69,"context_line":"            # for future compatibility."},{"line_number":70,"context_line":"            if err:"},{"line_number":71,"context_line":"                err_messages \u003d err.splitlines(False)"},{"line_number":72,"context_line":"                if err_messages !\u003d self.IGNORED_ERROR_MSG_SEQUENCE:"},{"line_number":73,"context_line":"                    raise exception(err)"},{"line_number":74,"context_line":"        except OSError:"},{"line_number":75,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_d3d01fca","line":72,"in_reply_to":"3a961159_5cd610c4","updated":"2015-01-17 03:01:54.000000000","message":"It\u0027s not out of the question, but I haven\u0027t seen anything which indicates the order of stderr is of great importance. It\u0027s usually just a stream of debug data. I believe it is safe to iter through the lines and whitelist the values you know are safe.\n\nSomething else to consider for your next patch is pushing this capability down into the base RestoreRunner. Maybe not so much the raising an exception for stderr messages, but at least making stderr available for the post_* functions in backups and restores.  The MySQL innobackupex runner also inspects the stderr stream because, oddly enough, that\u0027s where the success message for the backup tools is printed. (https://github.com/openstack/trove/blob/master/trove/guestagent/strategies/backup/mysql_impl.py#L69)\n\nI think it would be valuable to have stderr information from the process available. It would also prevent the need for re-inventing the process execution logic in the PostgreSQL restore runner.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":14576,"name":"Petr Malik","email":"pmalik@tesora.com","username":"pmalik"},"change_message_id":"a259ff18144bd09db8c2576d261a6cc164891ef3","unresolved":false,"context_lines":[{"line_number":69,"context_line":"            # for future compatibility."},{"line_number":70,"context_line":"            if err:"},{"line_number":71,"context_line":"                err_messages \u003d err.splitlines(False)"},{"line_number":72,"context_line":"                if err_messages !\u003d self.IGNORED_ERROR_MSG_SEQUENCE:"},{"line_number":73,"context_line":"                    raise exception(err)"},{"line_number":74,"context_line":"        except OSError:"},{"line_number":75,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_5cd610c4","line":72,"in_reply_to":"3a961159_88684f22","updated":"2015-01-16 16:40:16.000000000","message":"This is a tricky one and I\u0027d appreciate any feedback from somebody with more Postgres experience. I see what you mean. Having to ignore error messages smells bad enough to me... I just wanted to narrow the ignored scenario as much as possible to avoid false negatives. I didn\u0027t want to ignore any sequence of errors, but only this particular one.\nI guess, the question is: if there can be more messages in this sequence do they always appear in the same sequence? If they do, is it safe to ignore them even if they appear out of the expected sequence? Thanks for suggestions.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"},{"author":{"_account_id":14576,"name":"Petr Malik","email":"pmalik@tesora.com","username":"pmalik"},"change_message_id":"2618c4aada95ef339a39d0e0ab4a8f6d4f7428f9","unresolved":false,"context_lines":[{"line_number":69,"context_line":"            # for future compatibility."},{"line_number":70,"context_line":"            if err:"},{"line_number":71,"context_line":"                err_messages \u003d err.splitlines(False)"},{"line_number":72,"context_line":"                if err_messages !\u003d self.IGNORED_ERROR_MSG_SEQUENCE:"},{"line_number":73,"context_line":"                    raise exception(err)"},{"line_number":74,"context_line":"        except OSError:"},{"line_number":75,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_c7211dff","line":72,"in_reply_to":"3a961159_d3d01fca","updated":"2015-01-19 16:05:10.000000000","message":"OK, I take your suggestion and update the error handling code.\n\nGood idea on the code reuse, thanks. I will take a look at it.","commit_id":"7784c8eda00bcf946a191f6b549df5217d5c87e3"}]}