Alon Bar-Lev has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 4: (4 comments) hi saggi, some responses. any reason you do not review the latest revision? thanks, 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: > I don't like py.in files. 1. there is single py.in file which is config, just like config.h for autoconf. Unlike in vdsm there is no logic within it. 2. what we do in devenv is injecting alternate python dir and prepend it to the python module path so that the developer's artifacts will be read instead of system provided ones. 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 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 I must ask... as you commented a lot of this... why is it important? Line 180: 'NextKdumpTimeout', Line 181: 'KdumpFinishedTimeout', Line 182: 'FenceKdumpListenerHost', Line 183: 'FenceKdumpListenerPort', 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): > Having a base object to inherit just because is nothing short of ridiculous why do we need to setup logger for each class and duplicate the code? what is the alternative? calling a function at each constructor? will it make any difference? 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/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 ( > The whole point of the abstract 80 line limit is to eliminate excessive cod the 80 line limit began from the first punch card reader line size[1] it has nothing to do with software, not expression size.... :) inventing names for sub expressions makes each developer creativity turn in, and have the same condition looks different every time for every instance of same or other developer as people are inconsistent even with them-selves. but please review the last revision. [1] http://en.wikipedia.org/wiki/Punched_card 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-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
