feat: remove confluent-kafka as a hard dependency #26
Conversation
| from confluent_kafka import DeserializingConsumer, KafkaError | ||
| from confluent_kafka.schema_registry.avro import AvroDeserializer | ||
| except: | ||
| DeserializingConsumer = None |
There was a problem hiding this comment.
Consider:
confluent_kafka = None
Would the code read better if everything just checked:
if confluent_kafka:
That's what we do for New Relic.
There was a problem hiding this comment.
Good call. Will go with that.
There was a problem hiding this comment.
@robrap do you know of any ADRs or OEPs detailing how the decision was made to use this pattern for New Relic? Github code search is not forthcoming with any, but it'd be a useful reference if it exists.
There was a problem hiding this comment.
No ADR. I think the code just reads cleanly. See example: https://github.com/openedx/edx-django-utils/blob/master/edx_django_utils/monitoring/internal/middleware.py#L160-L161. I think it would be reasonable to note in this ADR the pattern of code that we'd follow.
rgraber
force-pushed
the
rsgraber/conditional-imports
branch
from
8d5347e
to
0b482fa
Compare
| Then, later on, before any usage of `DeserializingConsumer`:: | ||
|
|
||
| if not confluent_kafka: | ||
| warn("Confluent_kafka not installed") |
There was a problem hiding this comment.
Shouldn't we warn during import, if at all? Having lots of repeated warnings would be annoying for those that have no intent on using the feature. Or, is this only run by code that is clearly meant to be using the feature?
There was a problem hiding this comment.
I do like the idea that this only warns on import, but the examples below use this pattern where the warning shows up when we try to use confluent_kafka. I'll leave things the way it is now, and then make a decision about this a bit later.
There was a problem hiding this comment.
This is only run by code that tries to use the feature. My worry is that, depending on usage, the import warning might be far away from the failure that will happen when someone tries to actually use the code.
Co-authored-by: Tim McCormack <tmccormack@edx.org>
dianakhuang
force-pushed
the
rsgraber/conditional-imports
branch
from
cd6d3e6
to
da508a7
Compare
dianakhuang
force-pushed
the
rsgraber/conditional-imports
branch
from
da508a7
to
b50e795
Compare
Description:
Remove confluent-kafka as a dependency in base.in. Instead, wrap all imports in a try...catch and log error messages if the library is not found.
Issue:
openedx/openedx-events#88
Installation instructions:
As of this PR, any service that uses event-bus-kafka will need to independently install confluent-kafka.
Testing instructions:
Follow the instructions in manual_testing.rst EXCEPT for the bit about installing confluent-kafka.
You should get
Library confluent-kafka not available. Cannot create event producer.(or consumer).In the cms shell,
pip install confluent-kafkaYou should then be able to go through manual_testing again and get the correct results.
make testin the event-bus-kafka repo itself should always work (without having to install confluent-kafka separately) since confluent-kafka is in the test dependencies.Merge checklist:
Post merge:
finished.
Author concerns:
Deviated a bit from @robrap 's example of newrelic, where everything is in an
if newrelicblock. Instead, I used more early returns since I find that easier to read (rather than indenting entire methods). This does have the downside of making coverage.py upset since I have not been able to unit test those blocks. I'm not yet certain whether it's possible or whether we should justpragma: no-coverit or whether we should ignore it because the coverage check isn't mandatory.