[KIP-932] Add deserializing share consumer#2265
[KIP-932] Add deserializing share consumer#2265Kaushik Raina (k-raina) wants to merge 5 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Pranav Rathi (pranavrth)
left a comment
There was a problem hiding this comment.
I have provided few comments. Please fix those and then I will review this PR again.
| topic = msg.topic() | ||
| if topic is None: | ||
| # no topic to deserialize against; flag it rather than raise | ||
| msg.set_error(KafkaError(KafkaError._VALUE_DESERIALIZATION, "Message topic is None")) | ||
| return |
There was a problem hiding this comment.
This should be a TypeError. Check Deserializing Consumer and make sure we are following those conventions.
There was a problem hiding this comment.
I think we should write integration tests with Avro, Protobuf and JSONSchema as well. A positive case and an error case with each is good enough to make sure that everything is fine.
| if conf is not None: | ||
| share_conf.update(conf) | ||
|
|
||
| reset = share_conf.pop('auto.offset.reset', 'earliest') |
There was a problem hiding this comment.
This is incorrect use of the share consumer. Let's fix this even in above share consumer scenario and extract a common private function to reuse the same code.
| def set_headers(self, headers: HeadersType) -> None: ... | ||
| def set_key(self, key: Any) -> None: ... | ||
| def set_value(self, value: Any) -> None: ... | ||
| def set_error(self, error: KafkaError) -> None: ... |
There was a problem hiding this comment.
Do we really want to expose this function to the user? If yes then we have to add error handling in the case when the user is sending not error in the error field.
fb01516 to
3765147
Compare
d600e92 to
5e41108
Compare
3765147 to
7087ef4
Compare
|
Below are known test failures fixed in PR https://github.com/confluentinc/confluent-kafka-python/pull/2253/changes#top |
| if (new_error == Py_None) { | ||
| /* None clears the error; Message stores "no error" as NULL | ||
| * (Message_error then returns None for it). */ | ||
| self->error = NULL; |
There was a problem hiding this comment.
We never set error to NULL in python. It should be KafkaError or it should be None.
| if (new_error != Py_None && | ||
| !PyObject_TypeCheck(new_error, &KafkaErrorType)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "error must be a KafkaError or None"); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Add this breaking change in Changelog as well and add a TODO.
|
|
||
| topic = msg.topic() | ||
| if topic is None: | ||
| raise TypeError("Message topic is None") |
There was a problem hiding this comment.
Add TODO to verify if this is correct or not for GA.
We have 3 options:
- TypeError - but user didn't give the topic name so I think TypeError is not correct.
- RuntimeError - I feel this is correct as this is internal issue not related to user.
- KafkaError - set in the message field so that we don't throw exception.
| """ | ||
|
|
||
| messages = super(DeserializingShareConsumer, self).poll(timeout) | ||
| for msg in messages: |
There was a problem hiding this comment.
We need to check if we want to send error or exception in a message. We should use the pythonic way. Let's add a TODO to discuss it in the final interfaces part.
| raise TypeError("Message topic is None") | ||
|
|
||
| try: | ||
| ctx = SerializationContext(topic, MessageField.VALUE, msg.headers()) |
There was a problem hiding this comment.
Remove this to outside as it is being used below as well.
There was a problem hiding this comment.
This is not done.
| if topic is None: | ||
| raise TypeError("Message topic is None") | ||
|
|
||
| try: |
There was a problem hiding this comment.
Move try to after if self._value_deserializer is not None: part.
Pranav Rathi (pranavrth)
left a comment
There was a problem hiding this comment.
1 comment is missed.
| raise TypeError("Message topic is None") | ||
|
|
||
| try: | ||
| ctx = SerializationContext(topic, MessageField.VALUE, msg.headers()) |
There was a problem hiding this comment.
This is not done.
2fabcb1 to
2dce544
Compare
Pranav Rathi (pranavrth)
left a comment
There was a problem hiding this comment.
Looks good to me. I am approving the PR but I would prefer a pass from Robert Yokota (@rayokota) or somebody from the @confluentinc/data-governance team to review this as well.
|


Summary
key.deserializer/value.deserializerconfig (same callables as DeserializingConsumer).if msg.error():check and acknowledge with REJECT.confluent_kafkaand adds the missingMessage.set_errorto the cimpl type stub.