sdk-csharp icon indicating copy to clipboard operation
sdk-csharp copied to clipboard

feat: Allow empty payloads in Kafka, in order to support compaction tombstones

Open mhelleborg opened this issue 2 years ago • 2 comments

In order to support deletes for compacted topics, this PR allows empty / null payloads to be sent when using the Kafka extension. The previous requirement that the payload is present has been dropped, and tests added.

Ref Log compaction

mhelleborg avatar Jul 26 '22 09:07 mhelleborg

I'll have a look at this when I get the chance, but I'm afraid it may be a while due to vacations. (I don't know anything about Kafka, which means it'll take me longer to try to work out whether I think it's a good idea or not...)

jskeet avatar Jul 26 '22 10:07 jskeet

Thanks Jon :)

The change should make it easy to use CloudEvents for projecting state using compacted topics, as a streaming KV-store.

The zero-length payloads are treated as tombstones, and lets Kafka clean up removed messages over time.

mhelleborg avatar Jul 26 '22 10:07 mhelleborg

I still don't know why we prohibited empty messages in the first place. I'm going to ask at the meeting tomorrow. If other SDKs are fine with empty payloads, we should be too. If they're not, I want to find out why :)

jskeet avatar Aug 17 '22 09:08 jskeet

Commenting after the WG discussion.

I agree with this PR.

In general, Kafka records with a null value are valid records with both log compacted topics or with regular topics.

However, in the log compacted topics case, a record with a null value has a different semantic (tombstone), they signal to Kafka that it can remove all previous records with the same key but it retains and delivers to subscribers the tombstone record as long as it is consumed before the the topic-level configuration delete.retention.ms expires.

Since configuring a topic to be log compacted is an event producer's responsibility, the producer is specifically asking for that behavior so I don't see why it shouldn't be allowed to send a CloudEvent that is a tombstone given that:

  • the event itself is just a normal CloudEvent
  • the event producer is specifically asking the transport to have that side effect

pierDipi avatar Aug 18 '22 17:08 pierDipi

Just to set expectations, I'm hoping to merge this PR next week - I'm on vacation tomorrow, and done with work for today.

jskeet avatar Aug 18 '22 17:08 jskeet