Alon Bar-Lev has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 4: (6 comments) Please reply to the offline thread regarding the design pattern. As I wrote previously a simple single threaded timeout based pattern should be sufficient in this case and much simpler than any of the alternatives you proposed. 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 an you are not aware of devenv environment of engine, this is common for all, to be used in dual mode for unprivileged and production. 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. we use _ for the entire engine as C convention for gettext, this is the default of gettext for python as well. Line 21: Line 22: Line 23: @logbase.Logable() Line 24: class Manager(object): 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. for debug you are correct. for info and above that are exposed to user we use gettext, hence the format is better as it is standard. Line 114: db=self._database, Line 115: stmt=statement, Line 116: args=args, Line 117: ), 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 funct please look at other implementation within ovirt-engine code base. 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 on the opposite, throughout the code base of engine we eliminate the temporary variable use in favour of structured expressions. 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 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 again, I disagree, this convention is used all over the engine python code base, and was found to be productive. 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
