)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"eebb67593a2ccbc06d6310c302eb5263a92e4395","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Add support to OVSDB Connection class to filter IDL schema by columns,"},{"line_number":10,"context_line":"within tables, and not only by entire tables."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I292dbedf292557b12336eb1af5cfa6909159e70e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7aa08908_b03cd72f","line":11,"updated":"2016-06-14 15:39:03.000000000","message":"Any reference to why this might be needed? bug, blueprint?","commit_id":"440738b6aa600a434458c5df2a8ce426a287da4e"}],"neutron/agent/ovsdb/native/connection.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"3b5b3367e589de198995f96abcbf9f8562a8b7e0","unresolved":false,"context_lines":[{"line_number":61,"context_line":"        self.lock \u003d threading.Lock()"},{"line_number":62,"context_line":"        self.schema_name \u003d schema_name"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def start(self, table_name_list\u003dNone, table_name_columns\u003d{}):"},{"line_number":65,"context_line":"        \"\"\""},{"line_number":66,"context_line":"        :param table_name_list: A list of table names for schema_helper to"},{"line_number":67,"context_line":"                register. When this parameter is given, schema_helper will only"}],"source_content_type":"text/x-python","patch_set":1,"id":"1aa78d24_8d020a71","line":64,"updated":"2016-07-26 05:29:46.000000000","message":"It\u0027s generally bad practice to pass a mutable object as a default argument initializer. It creates a hidden global object and if it is ever modified, successive calls get broken. See: http://docs.python-guide.org/en/latest/writing/gotchas/\n\nNow, in this it isn\u0027t likely to be a problem since these functions are only called once. But it\u0027s an easy enough thing to avoid by just defaulting to None and passing:\n\n  table_name_columns or {}\n\nor something similar.","commit_id":"440738b6aa600a434458c5df2a8ce426a287da4e"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"131cfed46922805b6cb277c08132263a041bd5fd","unresolved":false,"context_lines":[{"line_number":61,"context_line":"        self.lock \u003d threading.Lock()"},{"line_number":62,"context_line":"        self.schema_name \u003d schema_name"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    def start(self, table_name_list\u003dNone, table_name_columns\u003d{}):"},{"line_number":65,"context_line":"        \"\"\""},{"line_number":66,"context_line":"        :param table_name_list: A list of table names for schema_helper to"},{"line_number":67,"context_line":"                register. When this parameter is given, schema_helper will only"}],"source_content_type":"text/x-python","patch_set":1,"id":"dada55a8_db146e45","line":64,"in_reply_to":"1aa78d24_8d020a71","updated":"2016-07-26 14:17:20.000000000","message":"Done","commit_id":"440738b6aa600a434458c5df2a8ce426a287da4e"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"3b5b3367e589de198995f96abcbf9f8562a8b7e0","unresolved":false,"context_lines":[{"line_number":85,"context_line":"            self.thread.setDaemon(True)"},{"line_number":86,"context_line":"            self.thread.start()"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    def get_schema_helper(self, table_name_list, table_name_columns):"},{"line_number":89,"context_line":"        \"\"\"Retrieve the schema helper object from OVSDB\"\"\""},{"line_number":90,"context_line":"        try:"},{"line_number":91,"context_line":"            helper \u003d idlutils.get_schema_helper(self.connection,"}],"source_content_type":"text/x-python","patch_set":1,"id":"dada55a8_9354a4fe","line":88,"updated":"2016-07-26 05:29:46.000000000","message":"I\u0027m not particularly fond of passing, to two different functions, a list and a dictionary where the keys are going to be the same as the list. I\u0027ve spent a lot of time trying to figure out what I\u0027d like better, and the best I\u0027ve got so far is if we added an optional schema_helper argument to __init__, and did self.schema_helper \u003d schema_helper or self._get_schema_helper() and made update_schema_helper an idlutils function since it doesn\u0027t require any instance or class data?\n\nLooking at the OVN code, it looks like overriding the IDL might be an even better option? Maybe a variation of this patch I wrote a long time ago that was abandoned because nothing actually needed the feature at the time? https://review.openstack.org/#/c/175062/4/neutron/agent/ovsdb/native/connection.py","commit_id":"440738b6aa600a434458c5df2a8ce426a287da4e"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"131cfed46922805b6cb277c08132263a041bd5fd","unresolved":false,"context_lines":[{"line_number":85,"context_line":"            self.thread.setDaemon(True)"},{"line_number":86,"context_line":"            self.thread.start()"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    def get_schema_helper(self, table_name_list, table_name_columns):"},{"line_number":89,"context_line":"        \"\"\"Retrieve the schema helper object from OVSDB\"\"\""},{"line_number":90,"context_line":"        try:"},{"line_number":91,"context_line":"            helper \u003d idlutils.get_schema_helper(self.connection,"}],"source_content_type":"text/x-python","patch_set":1,"id":"dada55a8_78be11af","line":88,"in_reply_to":"dada55a8_9354a4fe","updated":"2016-07-26 14:17:20.000000000","message":"This patch sits on top of https://review.openstack.org/#/c/323707/ . The thought in the original patch is to allow overriding this class. Implementation may then update the schema helper using different methods, or providing the schema helper independently. This is the reason that update_schema_helper is a part of this class, and public.\n\nI agree that the current parameter configuration is not optimal, but I wanted the API of \u0027start\u0027 to be backwards compatible. Passing the parameter to __init__ in this case is possible (and then we can do it without the list, only the dictionary), but that would change the paradigm used up till now, where the filter is passed to \u0027start\u0027. (It will also allow to methods to pass the filter, if we keep backwards compatibility)","commit_id":"440738b6aa600a434458c5df2a8ce426a287da4e"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9313eb19bffe9680e026ab1f697bf4540f404d6b","unresolved":false,"context_lines":[{"line_number":118,"context_line":""},{"line_number":119,"context_line":"    def update_schema_helper(self, helper,"},{"line_number":120,"context_line":"                             table_name_list, table_name_columns):"},{"line_number":121,"context_line":"        if table_name_list is None:"},{"line_number":122,"context_line":"            helper.register_all()"},{"line_number":123,"context_line":"        else:"},{"line_number":124,"context_line":"            if table_name_columns is None:"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_d63313f5","line":121,"updated":"2016-08-12 15:59:47.000000000","message":"It would be good to at least do:\n\n if table_name_list is None and table_name_columns:\n     table_name_list \u003d table_name_columns.keys()\n\nso that people don\u0027t have to pass both columns.keys() and columns in start(). Could default both to None in get/update as well.","commit_id":"e9bffba52e7a0b16ea4ca9435ceeb3acb3f7f6c7"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":26,"context_line":"from neutron.agent.ovsdb.native import idlutils"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"ALL_COLUMNS_CONSTANT \u003d \u0027*\u0027"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class TransactionQueue(Queue.Queue, object):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_7260e401","line":29,"updated":"2016-08-17 14:37:31.000000000","message":"Since constants are, by pep8 convention, module-level and all constants (https://www.python.org/dev/peps/pep-0008/#constants) having _CONSTANT is redundant.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"06adc41939f7f10074e7f0236c7aa1e1df58fefa","unresolved":false,"context_lines":[{"line_number":26,"context_line":"from neutron.agent.ovsdb.native import idlutils"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"ALL_COLUMNS_CONSTANT \u003d \u0027*\u0027"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class TransactionQueue(Queue.Queue, object):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_bfb7c4d6","line":29,"in_reply_to":"3ac371cc_7260e401","updated":"2016-08-17 14:48:53.000000000","message":"and all capitals, that is.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":26,"context_line":"from neutron.agent.ovsdb.native import idlutils"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"ALL_COLUMNS_CONSTANT \u003d \u0027*\u0027"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class TransactionQueue(Queue.Queue, object):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_226c5b34","line":29,"in_reply_to":"3ac371cc_bfb7c4d6","updated":"2016-08-17 15:12:18.000000000","message":"Will remove.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":118,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":119,"context_line":"                sfilter(helper)"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":122,"context_line":"        self.schema_filter.append("},{"line_number":123,"context_line":"            lambda helper: helper.register_table(*args, **kwargs)"},{"line_number":124,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_47c9ac58","line":121,"updated":"2016-08-17 14:37:31.000000000","message":"We should probably either document that the table argument should be passed or add the argument to the definition/lambda. I\u0027m ok with either.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":118,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":119,"context_line":"                sfilter(helper)"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":122,"context_line":"        self.schema_filter.append("},{"line_number":123,"context_line":"            lambda helper: helper.register_table(*args, **kwargs)"},{"line_number":124,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_42dd17df","line":121,"in_reply_to":"3ac371cc_47c9ac58","updated":"2016-08-17 15:12:18.000000000","message":"I\u0027ll add the documentation. The plan here is to be as stable as transparent as possible to the Idl API.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":123,"context_line":"            lambda helper: helper.register_table(*args, **kwargs)"},{"line_number":124,"context_line":"        )"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def register_columns(self, *args, **kwargs):"},{"line_number":127,"context_line":"        self.schema_filter.append("},{"line_number":128,"context_line":"            lambda helper: helper.register_columns(*args, **kwargs)"},{"line_number":129,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_27e590db","line":126,"updated":"2016-08-17 14:37:31.000000000","message":"Same","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":123,"context_line":"            lambda helper: helper.register_table(*args, **kwargs)"},{"line_number":124,"context_line":"        )"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def register_columns(self, *args, **kwargs):"},{"line_number":127,"context_line":"        self.schema_filter.append("},{"line_number":128,"context_line":"            lambda helper: helper.register_columns(*args, **kwargs)"},{"line_number":129,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_a2f4cb5a","line":126,"in_reply_to":"3ac371cc_27e590db","updated":"2016-08-17 15:12:18.000000000","message":"Same response :)","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def register_table_dictionary(self, schema_filter_dict):"},{"line_number":132,"context_line":"        for table, columns in six.iteritems(schema_filter_dict):"},{"line_number":133,"context_line":"            if columns and columns !\u003d ALL_COLUMNS_CONSTANT:"},{"line_number":134,"context_line":"                self.register_columns(table, columns)"},{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_cd95bd33","line":133,"updated":"2016-08-17 14:37:31.000000000","message":"Optional: Seeing this, if we just made {\u0027table1\u0027: []} (or None) be the format for no restrictions, we wouldn\u0027t need to check if columns and columns !\u003d ALL_COLUMNS.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"0a4f8fac6c4689773bc7aecc2d24592e2e5f3cdc","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def register_table_dictionary(self, schema_filter_dict):"},{"line_number":132,"context_line":"        for table, columns in six.iteritems(schema_filter_dict):"},{"line_number":133,"context_line":"            if columns and columns !\u003d ALL_COLUMNS_CONSTANT:"},{"line_number":134,"context_line":"                self.register_columns(table, columns)"},{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_55edcfb5","line":133,"in_reply_to":"3ac371cc_427997c7","updated":"2016-08-17 15:17:35.000000000","message":"Works for me.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def register_table_dictionary(self, schema_filter_dict):"},{"line_number":132,"context_line":"        for table, columns in six.iteritems(schema_filter_dict):"},{"line_number":133,"context_line":"            if columns and columns !\u003d ALL_COLUMNS_CONSTANT:"},{"line_number":134,"context_line":"                self.register_columns(table, columns)"},{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_427997c7","line":133,"in_reply_to":"3ac371cc_cd95bd33","updated":"2016-08-17 15:12:18.000000000","message":"True, but I wanted a constant that explicitly states \u0027all columns\u0027. It will be easier to read on the client code.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def register_table_names(self, table_name_list):"},{"line_number":139,"context_line":"        for table in table_name_list:"},{"line_number":140,"context_line":"            self.register_table(table)"},{"line_number":141,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_8d8bc567","line":138,"updated":"2016-08-17 14:37:31.000000000","message":"Optional: If we have register_table, it seems like this could just be register_tables.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def register_table_names(self, table_name_list):"},{"line_number":139,"context_line":"        for table in table_name_list:"},{"line_number":140,"context_line":"            self.register_table(table)"},{"line_number":141,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_a2504b33","line":138,"in_reply_to":"3ac371cc_8d8bc567","updated":"2016-08-17 15:12:18.000000000","message":"I wanted to use a different naming convention, since the implementation is very different (the above is a complete pass-through, and here we require a list of table names).\n\nI will add documentation here as well.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"0a4f8fac6c4689773bc7aecc2d24592e2e5f3cdc","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            else:"},{"line_number":136,"context_line":"                self.register_table(table)"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"    def register_table_names(self, table_name_list):"},{"line_number":139,"context_line":"        for table in table_name_list:"},{"line_number":140,"context_line":"            self.register_table(table)"},{"line_number":141,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_950aa788","line":138,"in_reply_to":"3ac371cc_a2504b33","updated":"2016-08-17 15:17:35.000000000","message":"Well, it\u0027s complete pass-through but unless they completely break their API (and thus this function), it will always have a table name as the first argument. In any case, not a big deal to me. It\u0027s just opinion. :)","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":6854,"name":"YAMAMOTO Takashi","email":"yamamoto@midokura.com","username":"yamamoto"},"change_message_id":"e5ac80d75ec70ff7b1ea3328432003f616eea293","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_d01f8007","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"updated":"2016-10-18 09:56:45.000000000","message":"these all register_xxx methods are just to set up self.schema_filter, right?\ni suppose it\u0027s cleaner to make the filter a separate class and make start and update_schema_helper take it as an argument.\nhow do you think?","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"f288033034a4f8fbf20b17276d71bbab72136d17","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_0a225c01","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"in_reply_to":"fa6399be_742661bb","updated":"2016-10-19 17:11:48.000000000","message":"As I mentioned in review 323707, these methods exist to allow extending the class.\n\nLet me think over the API a bit more. I\u0027ll see if I can come up with a better solution.","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"2648710c1c6f8e311aefd8279c9b50466a3f4c90","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_f54fc8bb","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"in_reply_to":"fa6399be_742661bb","updated":"2016-10-19 19:06:05.000000000","message":"It seems like the only difference in use would be:\n\n c \u003d Connection(...)\n filter \u003d SchemaFilter()\n filter.register_table(table)\n filter.register_columns(table, columns)\n c.start(..., filter)\n\nwith update_schema_helper taking an additional filter argument and basically turning into a @classmethod since self would no longer be used.\n\n c \u003d Connection(...)\n c.register_table(...)\n c.register_column(...)\n c.start()\n\nI originally suggested creating the concept of  a filter Command class here: https://gist.github.com/otherwiseguy/3bdc5566f3896bfffb9d5ab34d7e2736 just because _schema_filter being a list of functions that take an object on which arbitrary methods with arbitrary arguments are called is a little weird to work with.\n\nIf I had to do it all over again, knowing that we\u0027d want overidable Idl classes, and filters for the helper used to instantiate the Idl, I\u0027d just have the Connection class take an Idl instance. People could just pass in whatever they want and start would just start the loop. Maybe it would be worth just breaking backward compatibility to make things not suck. There aren\u0027t that many users of it, it\u0027d be easy to fix in OVN/dragonflow.","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"46d459151ae8168c4873b6ecad66294e87f21ac3","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_742661bb","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"in_reply_to":"fa6399be_93aeefe5","updated":"2016-10-19 16:09:42.000000000","message":"I agree with Yamamoto here. You can create the filter object in start and pass it to update_schema_helper. It\u0027s weird to have an update method where it\u0027s not clear what you are updating exactly since it\u0027s hidden in a self attribute.\n\n\u003eAn API change here will also affect https://review.openstack.org/#/c/323707 , since I want to keep the API the same once it\u0027s accepted. However, the concept of a filter object doesn\u0027t exist there.\n\nI don\u0027t think that is a valid reason to avoid improving the clarity of the code here :)","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"fe80b1dcab9bc77b81c306581b007033b7c57d9b","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_93aeefe5","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"in_reply_to":"fa6399be_d01f8007","updated":"2016-10-18 11:22:00.000000000","message":"I can change \u0027update_schema_helper\u0027 to receive a filter object. But I can\u0027t do that in \u0027start\u0027, since I don\u0027t want to change the API. I would rather also keep the meaning the same, so if a schema_filter list is passed to start, it will have to override the filter object.\n\nAn API change here will also affect https://review.openstack.org/#/c/323707 , since I want to keep the API the same once it\u0027s accepted. However, the concept of a filter object doesn\u0027t exist there.\n\nThis is one of the reason I separated the patches. I wanted one patch to allow Connection to be extended, and another patch to extend it (since I suspected there will be a discussion on exactly how to do it). If possible, I would like to keep design. However, this design means that parameters are passed via the class instance, which makes sense for subclasses.","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"5926432c66937281cd59c421cdb7f9643fe2a031","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            for sfilter in self.schema_filter:"},{"line_number":131,"context_line":"                sfilter(helper)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def register_table(self, *args, **kwargs):"},{"line_number":134,"context_line":"        \"\"\""},{"line_number":135,"context_line":"        Call register_table once this connection is started (by calling start"},{"line_number":136,"context_line":"        or update_schema_helper)."}],"source_content_type":"text/x-python","patch_set":8,"id":"fa6399be_9544cc74","line":133,"range":{"start_line":133,"start_character":8,"end_line":133,"end_character":22},"in_reply_to":"fa6399be_f54fc8bb","updated":"2016-10-19 19:11:37.000000000","message":"or, if we need to ensure that the Idl isn\u0027t instantiated until start() is called, then pass in a factory function.","commit_id":"b3b4d30bd81883af80638c713842daff70d88158"}],"neutron/tests/functional/agent/ovsdb/native/test_connection.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b38796d8dc7e8f7f77e1ac2bf6792eee4639814b","unresolved":false,"context_lines":[{"line_number":48,"context_line":"        self.connection.idl \u003d None"},{"line_number":49,"context_line":"        self.connection.register_table_dictionary(columns)"},{"line_number":50,"context_line":"        self.connection.start()"},{"line_number":51,"context_line":"        for table, cols in columns.items():"},{"line_number":52,"context_line":"            self.assertItemsEqual(cols,"},{"line_number":53,"context_line":"                                  self.connection.idl.tables[table].columns)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_a7b7c038","line":51,"updated":"2016-08-17 14:37:31.000000000","message":"Optional: Could throw in an assert on the length of the idl.tables to ensure that there are no extra tables registered.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"},{"author":{"_account_id":20229,"name":"Omer Anson","email":"omer.anson@toganetworks.com","username":"oanson"},"change_message_id":"452ca6c80ae3e441a0950d35f2f5cccbb8ae1341","unresolved":false,"context_lines":[{"line_number":48,"context_line":"        self.connection.idl \u003d None"},{"line_number":49,"context_line":"        self.connection.register_table_dictionary(columns)"},{"line_number":50,"context_line":"        self.connection.start()"},{"line_number":51,"context_line":"        for table, cols in columns.items():"},{"line_number":52,"context_line":"            self.assertItemsEqual(cols,"},{"line_number":53,"context_line":"                                  self.connection.idl.tables[table].columns)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_b53883a1","line":51,"in_reply_to":"3ac371cc_a7b7c038","updated":"2016-08-17 15:12:18.000000000","message":"Will do.","commit_id":"234841c6cec275467e5fcddf8551e8c827adce00"}],"neutron/tests/unit/agent/ovsdb/native/test_connection.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9313eb19bffe9680e026ab1f697bf4540f404d6b","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    def setUp(self):"},{"line_number":30,"context_line":"        super(TestOVSNativeConnection, self).setUp()"},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    def _assert_by_table_name_list_and_columns(self, helper, table_name_list,"},{"line_number":33,"context_line":"                                               table_name_columns):"},{"line_number":34,"context_line":"        reg_all_called \u003d table_name_list is None"},{"line_number":35,"context_line":"        reg_table_called \u003d False"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_28e695e8","line":32,"updated":"2016-08-12 15:59:47.000000000","message":"This whole test is essentially just duplicating the logic of the actual function, but more sloppily (just that we call register_table/column at least once based on args). To actually test the functionality correctly, one would essentially completely duplicate the logic of update_schema_helper, replacing the helper calls with mock.call(*args) and then use assert_has_calls() at the end. Just writing code twice isn\u0027t my favorite kind of testing.\n\nInstead, these should all be functional tests. A simple example would be:\n\n from oslo_config import cfg\n\n from neutron.agent.ovsdb.native import connection\n from neutron.tests.functional import base\n\n\n class OVSDBConnectionTestCase(base.BaseSudoTestCase):\n     def setUp(self):\n         super(OVSDBConnectionTestCase, self).setUp()\n         self.connection \u003d connection.Connection(\n             cfg.CONF.OVS.ovsdb_connection,\n             cfg.CONF.ovs_vsctl_timeout, \u0027Open_vSwitch\u0027)\n\n     def test_limit_tables(self):\n         tables \u003d [\u0027Open_vSwitch\u0027, \u0027Bridge\u0027, \u0027Port\u0027]\n         self.connection.start(table_name_list\u003dtables)\n         self.assertItemsEqual(tables, self.connection.idl.tables.keys())\n\n     def test_limit_columns(self):\n         columns \u003d {\u0027Open_vSwitch\u0027: [\u0027bridges\u0027, \u0027other_config\u0027],\n                    \u0027Bridge\u0027: [\u0027name\u0027]}\n         self.connection.start(table_name_columns\u003dcolumns)  # assumes table_name_list set as per other comment\n         for table, cols in columns.items():\n             self.assertItemsEqual(cols,\n                                   self.connection.idl.tables[table].columns)\n\nIf you felt the need to specifically test overriding get_schema_helper/update_schema_helper, you could create a subclass of Connection, but I don\u0027t really think it is that necessary given they are called by start(). In any case, all of these unit tests should go away.","commit_id":"e9bffba52e7a0b16ea4ca9435ceeb3acb3f7f6c7"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9313eb19bffe9680e026ab1f697bf4540f404d6b","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        self._test_start(table_name_list\u003d[\u0027fake-table1\u0027, \u0027fake-table2\u0027])"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    @helpers.requires_py2"},{"line_number":72,"context_line":"    def test_start_with_table_name_list_and_columns(self):"},{"line_number":73,"context_line":"        table_name_columns \u003d {"},{"line_number":74,"context_line":"            \u0027fake-table1\u0027: [\u0027fake-col1\u0027, \u0027fake-col2\u0027],"},{"line_number":75,"context_line":"            \u0027fake-table2\u0027: [\u0027fake-col3\u0027, \u0027fake-col4\u0027],"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_c898991e","line":72,"updated":"2016-08-12 15:59:47.000000000","message":"Might as well add a test where we pass a mix of tables and columns.","commit_id":"e9bffba52e7a0b16ea4ca9435ceeb3acb3f7f6c7"}]}