Fix bare except #279

edenhill · 2017-11-14T12:01:50Z

flake8 now warns on bare exception, this PR attempts to fix that.

jeffwidman · 2017-11-14T19:56:40Z

confluent_kafka/avro/cached_schema_registry_client.py

                # bad schema - should not happen
-                raise ClientError("Received bad schema from registry.")
+                raise ClientError("Received bad schema (id %s) from registry: %s" % (schema_id, str(e)))


Why cast to e to str? Why not rely on the implicit conversion?

To favour __str__ over __repr__, but I guess __str__ kicks in here thanks to %s anyway

jeffwidman · 2017-11-14T19:57:45Z

confluent_kafka/avro/cached_schema_registry_client.py

@@ -269,7 +270,8 @@ def test_compatibility(self, subject, avro_schema, version='latest'):
            else:
                log.error("Unable to check the compatibility")
                False
-        except:
+        except: # noqa


Shouldn't you at least restrict this to except Exception:?

That's what I thought too, until I found this:

In situations where you need to catch all “normal” errors, such as in a framework that runs callbacks, you can catch the base class for all normal exceptions, Exception. Unfortunately in Python 2.x it is possible for third-party code to raise exceptions that do not inherit from Exception, so in Python 2.x there are some cases where you may have to use a bare except: and manually re-raise the exceptions you don’t want to catch.

from https://docs.python.org/2/howto/doanddont.html#except

Are the exceptions being caught here from the app code or from this library (and other open-source libraries)?

If app code, that makes sense, but if from libraries, than I don't think it makes sense as generally most open source libs are well-behaved and if it is an issue, maintainers are generally very open to cleaning this up.

jeffwidman · 2017-11-14T19:59:25Z

confluent_kafka/avro/cached_schema_registry_client.py

@@ -269,7 +270,8 @@ def test_compatibility(self, subject, avro_schema, version='latest'):
            else:
                log.error("Unable to check the compatibility")
                False
-        except:
+        except: # noqa
+            log.error("_send_request() failed: %s" % sys.exc_info()[0])


Best practice is to use the logging framework's built-in string interpolation:

log.level("string %s", arg1)

That way the string isn't built unless the log level is hit. For error-level, this is simply a style thing, but for debug-level logging this can occasionally have a non-trivial performance impact.

Good input, thanks!

jeffwidman · 2017-11-14T20:00:43Z

confluent_kafka/avro/serializer/message_serializer.py

@@ -180,7 +179,7 @@ def _get_decoder_func(self, schema_id, payload):

                self.id_to_decoder_func[schema_id] = lambda p: read_data(p, schema_dict)
                return self.id_to_decoder_func[schema_id]
-            except:
+            except: # noqa


except Exception?

Thanks for the review!

edenhill · 2017-11-15T09:23:06Z

This should address all of Jeff's review points.

jeffwidman

looks good. caught a few things I missed before.

jeffwidman · 2017-11-15T22:16:03Z

confluent_kafka/avro/cached_schema_registry_client.py

@@ -269,7 +269,8 @@ def test_compatibility(self, subject, avro_schema, version='latest'):
            else:
                log.error("Unable to check the compatibility")
                False
-        except:
+        except Exception as e:
+            log.error("_send_request() failed: %s", e)


use log.exception()... it is the same as log.error() but it will automatically append the exception:

log.exception("_send_request() failed.")

jeffwidman · 2017-11-15T22:16:36Z

confluent_kafka/avro/load.py

+        else:
+            return schema.Parse(schema_str)
+    except schema.AvroException.SchemaParseException as e:
+        raise ClientError("Schema parse failed: %s" % (str(e)))


no need to cast to string...

jeffwidman · 2017-11-15T22:16:54Z

confluent_kafka/avro/serializer/message_serializer.py

-        except:
-            schema = None
+        except ClientError as e:
+            raise SerializerError("unable to fetch schema with id %d: %s" % (schema_id, str(e)))


no need to cast the exception to a string...

jeffwidman · 2017-11-15T22:18:26Z

tests/avro/mock_registry.py

        self.server.serve_forever()

+    def start(self):
+        """ Start, and wait for server to be fully started, before returning. """


nit: Per PEP-257, docstrings don't need space before/after the sentence.

This makes the tests more robust and faster

edenhill · 2017-12-07T15:46:38Z

Thanks for your review, @jeffwidman !

jeffwidman · 2017-12-08T03:20:46Z

No problem. Curious why you didn't implement the other points in my review of the fixes though? Was that purposeful decision or accidental oversight?

edenhill · 2017-12-08T08:18:20Z

@jeffwidman I thought I covered them all, what did I miss?

jeffwidman · 2017-12-08T09:08:35Z

Scroll here and you will see a few unaddressed comments: https://github.com/confluentinc/confluent-kafka-python/pull/279/files

No problem if you choose not to incorporate them, I mentioned only because I had a hunch you never even saw them.

edenhill requested a review from ewencp November 14, 2017 12:01

edenhill force-pushed the fix_bare_except branch from 975a407 to d8b3eda Compare November 14, 2017 12:02

jeffwidman reviewed Nov 14, 2017

View reviewed changes

ewencp requested a review from a team November 14, 2017 20:13

edenhill force-pushed the fix_bare_except branch from d8b3eda to a78bb20 Compare November 15, 2017 09:21

edenhill force-pushed the fix_bare_except branch from a78bb20 to d6b0f0f Compare November 15, 2017 09:29

jeffwidman approved these changes Nov 15, 2017

View reviewed changes

tburmeister mentioned this pull request Dec 7, 2017

first attempt at batch consumption of messages #282

Merged

edenhill added 2 commits December 7, 2017 16:24

Fix flake8 warnings, mainly bare exceptions

b181d3c

avro test: wait for mock server start rather than arbitrary sleep

efa47ae

This makes the tests more robust and faster

edenhill force-pushed the fix_bare_except branch from d6b0f0f to efa47ae Compare December 7, 2017 15:25

edenhill merged commit e9d7932 into master Dec 7, 2017

Fix bare except #279

Fix bare except #279

Uh oh!

Conversation

edenhill commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edenhill Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edenhill commented Nov 15, 2017

Uh oh!

jeffwidman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffwidman Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffwidman Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edenhill commented Dec 7, 2017

Uh oh!

jeffwidman commented Dec 8, 2017

Uh oh!

edenhill commented Dec 8, 2017

Uh oh!

jeffwidman commented Dec 8, 2017

Uh oh!

Uh oh!

edenhill Nov 14, 2017 •

edited

Loading

jeffwidman Nov 15, 2017 •

edited

Loading

jeffwidman Nov 15, 2017 •

edited

Loading