Saggi Mizrahi has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 4: (2 comments) 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 ( > I must ask... as you commented a lot of this... why is it important? In our experience in VDSM long try\except cause issues with pin pointing what exactly went wrong. Especially when you do stuff that are frowned upon like raising RuntimeException() instead of the original errors in some places. When you get an exception you know the error was somewhere in the try\except block. The smaller it is the easier it is to pin point the bug. In python, for each line you use you can get an error because of the dynamic nature of the language. We had cases where silly mistakes were thought to be other errors because of similar practices. Logging the original error removes some of the ambiguity but you can't trust you will always have debug level logs. It's also a matter of intent. try\except mean that you expect a certain part to fail. ```` a/b ```` and ```` try: a/b except ZeroDivisionError: .... ```` Are different. By wrapping something in try\except you are saying to the reader "I think this stuff will fail". By putting it over large blocks the reader misses the context. You want the cause of the error and the error handling to be as close to each other as possible. This is also why I asked to make the connection a context so that you can use the context over the loop but the try\except where you actually expect the error. to make context management and error handling more apparent. This is why python has the 'else' clause to handle cases where no exception happened but you are out of the woods. try: a = riskyOp() except: ... else: a / 0 There is also the matter of Exception types not being part of the interface in python (unlike java) but that is a story for another time. Real world example try: a = cache[key] a.update() except KeyError: a = cache[key] = load(key) # No need to update as it's freshly loaded Because of a spelling error in update() it now mistakenly throws KeyError. Because this has not effect on the result of the function (except for performance) and we don't really test for scale everything keeps working until a customer starts complaining. We had that happen multiple times in VDSM. try: a = cache[key] except KeyError: a = cache[key] = load(key) # No need to update as it's freshly loaded else: a.update() Would solve that issue. 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): > why do we need to setup logger for each class and duplicate the code? what No, I suggested doing this: def Logable(target): _logger = logging.getLogger(base.Base._LOG_PREFIX, ... Instead of a callable class. since it's a decorator and not a base class. I wouldn't have put _LOG_PREFIX on a class anyway as it's in no way specific to a class or a class hierarchy. Why is it self.__module__ and not target.__module__ anyway? Line 21: def __call__(self, target): Line 22: _logger = logging.getLogger(self._LOG_PREFIX + self.__module__) Line 23: setattr(target, "_logger", _logger) -- 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
