Bad code in CachedSchemaRegistryClient

[rhaarm]

Description

compatibility doesn't exist, so this could cause a exception, is that the purpose?

confluent-kafka-python/confluent_kafka/avro/cached_schema_registry_client.py

Line 314 in 7121757

if not compatibility:

This doesn't return, seems to be a bug, so the function would actually return None.

confluent-kafka-python/confluent_kafka/avro/cached_schema_registry_client.py

Line 271 in 7121757

False

What's the purpose of this exception catch? if you are going to raise it without logging or don't care?

confluent-kafka-python/confluent_kafka/avro/cached_schema_registry_client.py

Line 207 in 7121757

except ClientError:

How to reproduce

Just looking at code...

Checklist

Please provide the following information:

• confluent-kafka-python and librdkafka version (confluent_kafka.version() and confluent_kafka.libversion()):
• Apache Kafka broker version:
• Client configuration: {...}
• Operating system:
• Provide client logs (with 'debug': '..' as necessary)
• Provide broker log excerpts
• Critical issue

[edenhill]

Oooh, spontaneous code review, much appreciated!

1. You're right, it looks like compatibility should be initialized to None and the if check be changed to if compatibilty is None:. Without looking to deep it might be the case that _send_request() only returns values in the 2xx range, in which case that if-statement should be removed instead. Needs further investigation.

2. Right on! The log line should probably include the code too, for troubleshooting.

3. I believe this is just an attempt of being explicit of when schema loading might fail, which is fine. Code as docs.

Would you care to provide PRs for 1 and 2? :)

[stanislavkozlovski]

If not, I'm open for providing PRs ;)

[rnpridgeon]

@Enether, still up for submitting PRs for this? If not let me know and I'll go ahead and get these moving.

[rhaarm]

I won't be able to, but I just happened to come across these and wanted to point them out, while reviewing how the Schema Registry Client and Avro Producer work. The way the producer work, doesn't fit my situation since I'll be using multiple avro schemas per topic, I would prefer to not to manage 100s of topics for the same end goal.

…

On Thu, May 17, 2018 at 11:33 AM Ryan P ***@***.***> wrote: @Enether <https://github.com/Enether>, still up for submitting PRs for this? If not let me know and I'll go ahead and get these moving. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#372 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMZfLbcCwz5WuENmx1ut3qtmOkk6aMuks5tzaZugaJpZM4T4yvt> .

[stanislavkozlovski]

Yes I am. I'll have it completed this weekend. Is that okay, @rnpridgeon ?

[rnpridgeon]

That works great! Looking forward to your pull request @Enether

[stanislavkozlovski]

@edenhill

Without looking to deep it might be the case that _send_request() only returns values in the 2xx range, in which case that if-statement should be removed instead. Needs further investigation.

That appears to be false. _send_request() simply uses the requests library to make an HTTP call.
The requests library itself does not raise errors if the status code is not 2xx, but rather simply returns the response object.

import requests
print(requests.get('https://httpstat.us/200').status_code) # => 200
print(requests.get('https://httpstat.us/400').status_code) # => 400

cc: @rnpridgeon
Fixed with #379