Properly handle events without the data field
#1460
Conversation
knative-prow-robot
added
area/data-plane
size/L
|
/cherry-pick release-1.0 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.0 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-0.26 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.26 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-0.25 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.25 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-0.26 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.26 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-1.0 |
|
@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.0 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report
@@ Coverage Diff @@
## main #1460 +/- ##
============================================
- Coverage 73.41% 73.38% -0.04%
- Complexity 575 586 +11
============================================
Files 100 102 +2
Lines 3942 3986 +44
Branches 160 165 +5
============================================
+ Hits 2894 2925 +31
- Misses 808 823 +15
+ Partials 240 238 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
pierDipi
force-pushed
the
KNATIVE-1455_NPE-event-no-data
branch
from
aca7c28 to
8267d11
Compare
|
GH action failure could be ignored (ref: actions/checkout#273) |
| // A valid CloudEvent in the CE binary protocol binding of Kafka | ||
| // might be composed by only Headers. | ||
| // | ||
| // KafkaConsumer doesn't call the deserializer if the value | ||
| // is null. | ||
| // | ||
| // That means that we get a record with a null value event though | ||
| // the record is a valid CloudEvent. | ||
| if (record.value() == null) { | ||
| logDebug("Value is null", record); | ||
| final var value = cloudEventDeserializer.deserialize(record.record().topic(), record.record().headers(), null); | ||
| return new KafkaConsumerRecordImpl<>(KafkaConsumerRecordUtils.copyRecordAssigningValue(record.record(), value)); | ||
| } | ||
| return record; |
There was a problem hiding this comment.
buildCloudEventValueIfMissing
aren't we here "building" (deserializing) the value of the KafkaConsumerRecord, that happens to be a CloudEvent?
Could we change to if (record.value() != null) { return record; } else { ....} ? Not sure, but that might be easier to read
deserialize(record.record().topic(), record.record().headers(), null); I understand why we pass in null, but.... where is else is the deserialization done? why is it here again invoked, just b/c it is null 🤷♂️
There was a problem hiding this comment.
Could we change to if (record.value() != null) { return record; } else { ....} ? Not sure, but that might be easier to read
I agree!
deserialize(record.record().topic(), record.record().headers(), null); I understand why we pass in null, but.... where is else is the deserialization done? why is it here again invoked, just b/c it is null
The deserialization is done inside the KafkaConsumer, but when value == null, it doesn't call the deserializer since there is nothing to deserialize, so we need to deserialize the record manually when this happens since only record headers constitute a valid CloudEvent.
There was a problem hiding this comment.
but when
value == null, it doesn't call the deserializer
so that's OK for pure/vanilla kafka - but is that a problem for other "protocols" on top? Or is that just a corner-case here 🤷♂️
we need to deserialize the record manually when this happens since only record headers constitute a valid CloudEvent.
I just wonder how to make this more obvious - I understand we do it... perhaps the buildCloudEventValueIfMissing name is also not perfect ? 🙈 Sorry, just trying to make it "simpler"
There was a problem hiding this comment.
Sorry, just trying to make it "simpler"
Nah, we're doing code review for this reason, right? 😍
I changed the name to maybeDeserializeValueFromHeaders and changed the if .. as suggested.
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
pierDipi
force-pushed
the
KNATIVE-1455_NPE-event-no-data
branch
from
9e46b72 to
6f563e9
Compare
|
/lgtm
/approve
On Wed 17. Nov 2021 at 13:13, Pierangelo Di Pilato ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
data-plane/dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java
<#1460 (comment)>
:
> + // A valid CloudEvent in the CE binary protocol binding of Kafka
+ // might be composed by only Headers.
+ //
+ // KafkaConsumer doesn't call the deserializer if the value
+ // is null.
+ //
+ // That means that we get a record with a null value event though
+ // the record is a valid CloudEvent.
+ if (record.value() == null) {
+ logDebug("Value is null", record);
+ final var value = cloudEventDeserializer.deserialize(record.record().topic(), record.record().headers(), null);
+ return new KafkaConsumerRecordImpl<>(KafkaConsumerRecordUtils.copyRecordAssigningValue(record.record(), value));
+ }
+ return record;
Sorry, just trying to make it "simpler"
Nah, we're doing code review for this reason, right? 😍
I changed the name to maybeDeserializeValueFromHeaders and changed the if
.. as suggested.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1460 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABGPTUYAANY7H3RRJ57LSDUMOL7TANCNFSM5HYSBTZQ>
.
--
Sent from Gmail Mobile
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@pierDipi: #1460 failed to apply on top of branch "release-0.26": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@pierDipi: #1460 failed to apply on top of branch "release-0.25": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@pierDipi: new pull request created: #1500 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
) A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
) A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
) (knative-extensions#1502) A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com> Co-authored-by: Pierangelo Di Pilato <pdipilat@redhat.com>
) (knative-extensions#1502) (#39) A valid CloudEvent in the CE binary protocol binding of Kafka might be composed by only Headers. KafkaConsumer doesn't call the deserializer if the value is null. That means that we get a record with a null value even though the record is a valid CloudEvent. This patch handles events without the data field properly by creating the CloudEvent object from record headers, if the above conditions apply. Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
A valid CloudEvent in the CE binary protocol binding of Kafka
might be composed by only Headers.
KafkaConsumer doesn't call the deserializer if the value
is null.
That means that we get a record with a null value even though
the record is a valid CloudEvent.
This patch handles events without the data field properly
by creating the CloudEvent object from record headers, if
the above conditions apply.
Fixes #1455
Proposed Changes
Release Note
Docs