Hi Fede; > FV1: Looking at PayloadStore and PayloadResponse, "id" and "path" > seems to be the same thing, which is a reference to the data in the > external store. If my interpretation is correct, could we converge to > using one of the two terms? Path usually refers to the full path stored in Kafka topic as a full reference while an id is for file name for example `s3://my-bucket/my-topic/<random_uuid>` or `redis:cluster1:<key_id>`.
> FV2: I think PayloadResponse javadoc or PayloadStore interface may > need some updates. It says "Response from publish / download", but > PayloadStore.download returns a byte array, not a PayloadResponse. Updated the KIP now and removed PayloadResponse and used the PayloadStoreException as this seems to be enough now after all the previous simplification after the last update. > FV3: In PayloadStore interface, public and abstract are implicit > modifiers, so I think we can remove them. Updated now Thanks Omnia > On 26 Jul 2025, at 16:06, Federico Valeri <fedeval...@gmail.com> wrote: > > Hi Omnia, nice design. I have few questions: > > FV1: Looking at PayloadStore and PayloadResponse, "id" and "path" > seems to be the same thing, which is a reference to the data in the > external store. If my interpretation is correct, could we converge to > using one of the two terms? > > FV2: I think PayloadResponse javadoc or PayloadStore interface may > need some updates. It says "Response from publish / download", but > PayloadStore.download returns a byte array, not a PayloadResponse. > > FV3: In PayloadStore interface, public and abstract are implicit > modifiers, so I think we can remove them. > > Thanks > Fede > > On Fri, Jul 25, 2025 at 3:28 PM Omnia Ibrahim <o.g.h.ibra...@gmail.com > <mailto:o.g.h.ibra...@gmail.com>> wrote: >> >> Hi Jun, thanks for having the time to review this >> >>> JR1. While the KIP is potentially useful, I am wondering who is responsible >>> for retention for the objects in the payload store. Once a message with a >>> reference is deleted, the key of the external object is lost and the object >>> may never be deleted. >> >> The `ttl` in the object store is the responsibility of the owner of this >> store it should be configured in away that is reasonable with the retention >> config in Kafka. >> I have updated the KIP with `Consideration` section. >>> >>> JR2. Configs: For all new configs, it would be useful to list their types. >> Updated the KIP now >>> >>> JR3. value.serializers: Why is this required? If a user doesn't set it, we >>> should just use value.serializer, right? Ditto for key.serializers. >> No you right this was copy/past mistake >> >>> >>> JR4. For all new public interfaces such as LargeMessageSerializer, >>> PayloadStore and PayloadResponse, it would be useful to include the full >>> package name. >> Updated the KIP now >>> >>> JR5. large.message.payload.store.retry.max.backoff.ms and >>> large.message.payload.store.retry.delay.backoff.ms: Is the intention to >>> implement exponential backoff on retries? If so, it's more consistent if we >>> can follow the existing naming convention like retry.backoff.max.ms >>> <http://retry.backoff.max.ms/> <http://retry.backoff.max.ms/> and >>> retry.backoff.ms <http://retry.backoff.ms/> <http://retry.backoff.ms/>. >> I have removed these to simplify the config more (as Luke suggested >> initially) and added these to the consideration section. >> >>> >>> JR6. large.message.skip.not.found.error : If the reference can't be found, >>> what value does the deserializer return? Note that null has a special >>> meaning for tombstone in compacted topics. >> The deserialiser will return `new byte[0]` not null. >>> >>> JR7. PayloadResponse: Why do we have both responseCode and >>> PayloadStoreException? >> We can do without responseCode, the initial though was to report response >> code form payload store. >> Update the KIP. >>> JR8. Why do we need PayloadStore.metrics? Note that we could monitor the >>> metrics in a plugin through the Monitorable interface. >> Oh nice, I didn’t know about this interface before. Updated the KIP with >> this now. >>> >>> JR9. Why do we need the protected field PayloadStoreException.isRetryable? >> Initial thought here was the serializer can retry the upload. But I have >> removed all the retry logic from serializer and it will be up to the >> PayloadStore provider to implement this if they need it. >>> >>> JR10. As Luke mentioned earlier, we could turn PayloadStore to an interface. >> It is updated now to interface. >> >> Hope the last version of the KIP is more simpler now >> >> Thanks >> Omnia >> >>> On 23 Jul 2025, at 00:43, Jun Rao <j...@confluent.io.INVALID >>> <mailto:j...@confluent.io.INVALID>> wrote: >>> >>> Thanks for the KIP. A few comments. >>> >>> JR1. While the KIP is potentially useful, I am wondering who is responsible >>> for retention for the objects in the payload store. Once a message with a >>> reference is deleted, the key of the external object is lost and the object >>> may never be deleted. >>> >>> JR2. Configs: For all new configs, it would be useful to list their types. >>> >>> JR3. value.serializers: Why is this required? If a user doesn't set it, we >>> should just use value.serializer, right? Ditto for key.serializers. >>> >>> JR4. For all new public interfaces such as LargeMessageSerializer, >>> PayloadStore and PayloadResponse, it would be useful to include the full >>> package name. >>> >>> JR5. large.message.payload.store.retry.max.backoff.ms and >>> large.message.payload.store.retry.delay.backoff.ms: Is the intention to >>> implement exponential backoff on retries? If so, it's more consistent if we >>> can follow the existing naming convention like retry.backoff.max.ms >>> <http://retry.backoff.max.ms/> <http://retry.backoff.max.ms/> and >>> retry.backoff.ms <http://retry.backoff.ms/> <http://retry.backoff.ms/>. >>> >>> JR6. large.message.skip.not.found.error : If the reference can't be found, >>> what value does the deserializer return? Note that null has a special >>> meaning for tombstone in compacted topics. >>> >>> JR7. PayloadResponse: Why do we have both responseCode and >>> PayloadStoreException? >>> >>> JR8. Why do we need PayloadStore.metrics? Note that we could monitor the >>> metrics in a plugin through the Monitorable interface. >>> >>> JR9. Why do we need the protected field PayloadStoreException.isRetryable? >>> >>> JR10. As Luke mentioned earlier, we could turn PayloadStore to an interface. >>> >>> Thanks,
