Saggi Mizrahi has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 4: Code-Review-1 (24 comments) Some of my comments are written once but apply to all places that do the same thing. In general think that keeping a line short doesn't just mean using the "return" key. It supposed to help you realize that you might be doing to many things at once and maybe you should use helper variables. Don't hide exceptions unless they cause a fundamentally different error. If I get an error when opening a file and that is what I want the user to know went wrong I should just log and reraise. If not being able to open a file *means* something else, I will create a different error. To reiterate you usually wrap an error if the original error means something other than that error error (like a file not existing meaning that a service is down and not just the a file is not there). Also, the whole things looks a bit to complex, To many threads sleeping most of the time. You could just queue callbacks in a thread pool to empty the queue. Similarly, if you are trying to use a conveyor belt approach pushing events to the DB could\should be done using an batching system. That would remove the need for a DB manager as only one thing would ever have access to the DB and would simplify how everything works. Apart from that it appears like a lot of care and hard work went into making this. Good job. http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/__init__.py File packaging/services/ovirt-fence-kdump-listener/__init__.py: Line 1: # Copyright 2012 Red Hat 2014? Line 2: # Line 3: # Licensed under the Apache License, Version 2.0 (the "License"); Line 4: # you may not use this file except in compliance with the License. Line 5: # You may obtain a copy of the License at http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/config.py.in File packaging/services/ovirt-fence-kdump-listener/config.py.in: Line 10: # distributed under the License is distributed on an "AS IS" BASIS, Line 11: # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. Line 12: # See the License for the specific language governing permissions and Line 13: # limitations under the License. Line 14: EVerything in this file looks like something that environment variables and\or cmdline arguments should handle Line 15: Line 16: DEV_PYTHON_DIR = '@DEV_PYTHON_DIR@' Line 17: ENGINE_VARS = '@ENGINE_VARS@' Line 18: http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 16: import psycopg2 Line 17: Line 18: import logbase Line 19: Line 20: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-fence-kdump-listener') _ is usually used to mark variables are not needed. It might be that it's some sort of convention over their but for translation I would use 'tr' (like QT does). Line 21: Line 22: Line 23: @logbase.Logable() Line 24: class Manager(object): Line 108: ): Line 109: ret = [] Line 110: cursor = None Line 111: try: Line 112: self._logger.debug( Could be out of the try\except block Line 113: "Database: '{db}', Statement: '{stmt}', args: {args}".format( Line 114: db=self._database, Line 115: stmt=statement, Line 116: args=args, Line 109: ret = [] Line 110: cursor = None Line 111: try: Line 112: self._logger.debug( Line 113: "Database: '{db}', Statement: '{stmt}', args: {args}".format( I'm going to put this here but it's valid for all logging messages. In python you don't format the stings yourself you let the logger do it for you. This means that you don't get the formatting overhead if "debug" logging is turned off. self._logger.debug("blah %d %s", 2, "test") In general, prefer using the % operator to .format() for simple formatting as it doesn't have as much overhead. Line 114: db=self._database, Line 115: stmt=statement, Line 116: args=args, Line 117: ), Line 116: args=args, Line 117: ), Line 118: ) Line 119: Line 120: cursor = self._connection.cursor() Could be in it's own try\except block to stop checking if cursor is None Could also be pulled to a context so you could do: with self.getCursor() as c: c.foo() https://docs.python.org/2/library/contextlib.html#contextlib.contextmanager Line 121: cursor.execute( Line 122: statement, Line 123: args, Line 124: ) Line 131: break Line 132: ret.append(dict(zip(cols, entry))) Line 133: Line 134: except (psycopg2.Warning, psycopg2.Error) as e: Line 135: self._connection = None Shouldn't you be calling close_connection() so that you have a single place where you shut everything down. Line 136: self._logger.debug( Line 137: "Error executing statement.", Line 138: exc_info=True, Line 139: ) Line 175: def load_config_values(self): Line 176: config_values = {} Line 177: self.open_connection() Line 178: try: Line 179: for name in ( Could be outside of the try\except block Line 180: 'NextKdumpTimeout', Line 181: 'KdumpFinishedTimeout', Line 182: 'FenceKdumpListenerHost', Line 183: 'FenceKdumpListenerPort', Line 190: FROM vdc_options Line 191: WHERE option_name=%(name)s Line 192: AND version=%(version)s Line 193: """, Line 194: args=dict( Use dict literals. They are (arguably) more readable and are a lot faster. We actually had loops that this kind of code was the bottleneck for in VDSM Line 195: name=name, Line 196: version='general' Line 197: ), Line 198: ) Line 195: name=name, Line 196: version='general' Line 197: ), Line 198: ) Line 199: if not result: Could be outside of the try\except block Line 200: raise RuntimeError( Line 201: _('Cannot locate application option {name}').format( Line 202: name=name, Line 203: ) Line 203: ) Line 204: ) Line 205: Line 206: config_values[name] = result[0]['option_value'] Line 207: except (psycopg2.Warning, psycopg2.Error) as e: Seems to be a repeating pattern. Line 208: self._connection = None Line 209: self._logger.debug( Line 210: "Error loading config values.", Line 211: exc_info=True, Line 209: self._logger.debug( Line 210: "Error loading config values.", Line 211: exc_info=True, Line 212: ) Line 213: raise RuntimeError( I don't see a need to override the exception. It would only make the stack trace less useful. True for all such usages throughout the code Line 214: _('Error loading config values: {error}').format( Line 215: error=e, Line 216: ) Line 217: ) http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/flowterminator.py File packaging/services/ovirt-fence-kdump-listener/flowterminator.py: Line 23: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-fence-kdump-listener') Line 24: Line 25: Line 26: @logbase.Logable() Line 27: class KdumpFlowTerminator(threading.Thread): Inheriting from thread makes no sense. You are not creating a new type of thread. Also the responsibility of how to run something is the callers' and not the object. For example the caller could have just used a timer to run this at an interval in a thread pool or a new thread instead of having a thread that sleeps and needs to be taken care of. Line 28: """Creates record with status 'finished' for finished kdump flows""" Line 29: Line 30: def __init__( Line 31: self, Line 29: Line 30: def __init__( Line 31: self, Line 32: queue, Line 33: db_manager, db_manager should just be db. Line 34: kdump_finished_timeout, Line 35: flow_terminator_interval, Line 36: ): Line 37: super(KdumpFlowTerminator, self).__init__() Line 64: Line 65: def run(self): Line 66: while True: Line 67: try: Line 68: self._db_manager.open_connection() Make context as noted in the db file. Line 69: Line 70: finished = datetime.datetime.now( Line 71: dateutil.tz.tzutc() Line 72: ) Line 72: ) Line 73: records = self._get_unfinished_flows() Line 74: for record in records: Line 75: if ( Line 76: finished > ( Variable are free meaningful_name = record['received'] + self.kdump_finished_timeout if finished > meaningful_name: Line 77: record['received'] Line 78: + self.kdump_finished_timeout Line 79: ) Line 80: ): Line 77: record['received'] Line 78: + self.kdump_finished_timeout Line 79: ) Line 80: ): Line 81: msg = dict( literal dict as noted before Line 82: host=record['host'], Line 83: received=finished, Line 84: status='finished', Line 85: ) http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/heartbeat.py File packaging/services/ovirt-fence-kdump-listener/heartbeat.py: Line 23: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-fence-kdump-listener') Line 24: Line 25: Line 26: @logbase.Logable() Line 27: class Heartbeat(threading.Thread): Again, no inheriting from thread. Use timer. see flowterminator.py Line 28: """Periodically updates received to current time for record Line 29: with host 'fence-kdump-listener'""" Line 30: Line 31: def __init__(self, db_manager, alive_timeout): Line 50: ), Line 51: ), Line 52: ) Line 53: self._db_manager.commit() Line 54: except RuntimeError as e: Almost everything inherits from RutimeError. Look at my comments in db.py so that you have concrete exceptions to catch. Line 55: self._db_manager.rollback() Line 56: self._logger.error( Line 57: _("Error updating heartbeat: {error}").format( Line 58: error=e, http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 72: port, Line 73: queue, Line 74: ): Line 75: self._queue = queue Line 76: self._server = SocketServer.ThreadingUDPServer( Don't use ThreadingUDPServer. Especially as you already queue the hard work to another thread. Line 77: (host, port), Line 78: FenceKdumpRequestHandler, Line 79: ) Line 80: self._server.queue = self._queue http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/logbase.py File packaging/services/ovirt-fence-kdump-listener/logbase.py: Line 16: Line 17: from ovirt_engine import base Line 18: Line 19: Line 20: class Logable(base.Base): I don't know what base.Base is but it looks like this could just be a function. Line 21: def __call__(self, target): Line 22: _logger = logging.getLogger(self._LOG_PREFIX + self.__module__) Line 23: setattr(target, "_logger", _logger) http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 50: pidfile, Line 51: ): Line 52: # Check the required engine directories and files: Line 53: self.check( Line 54: os.path.join( Instead of making disjointed lines of complex statements just use variables. Line 55: self._config.get("ENGINE_USR"), Line 56: "services", Line 57: ), Line 58: directory=True, http://gerrit.ovirt.org/#/c/27201/4/packaging/services/ovirt-fence-kdump-listener/qconsumer.py File packaging/services/ovirt-fence-kdump-listener/qconsumer.py: Line 22: _ = lambda m: gettext.dgettext(message=m, domain='ovirt-fence-kdump-listener') Line 23: Line 24: Line 25: @logbase.Logable() Line 26: class QueueConsumer(threading.Thread): This is no name for a class. Doesn't help me understand why it's here. I know it consumes a queue but why? Line 27: """Receives messages from queue and stores them into database""" Line 28: Line 29: def __init__(self, queue, db_manager, next_kdump_timeout): Line 30: super(QueueConsumer, self).__init__() Line 77: self._db_manager.open_connection() Line 78: message = self._queue.get() Line 79: if message['status'] is None: Line 80: latest = self._get_latest(message['host']) Line 81: if ( This is unreadable variables are your friends Line 82: latest is None Line 83: or ( Line 84: message['received'] > ( Line 85: latest['received'] -- To view, visit http://gerrit.ovirt.org/27201 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieec3bad47bbba860a52a9ff4e2eb7f61277f4e36 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
