Clarification on TTL Change Logic in logstructured.go

[Yuyz0112]

Hi,

I am reviewing KINE’s code to understand its TTL implementation and came across the following snippet:
https://github.com/k3s-io/kine/blob/master/pkg/logstructured/logstructured.go#L338-L343

I noticed that the condition for entering the if block (TTL changed for key) is based on the value of preEventKV.expiredAt rather than curEventKV.expiredAt.

Could you please clarify why the comparison uses preEventKV.expiredAt instead of curEventKV.expiredAt? Wouldn’t it make more sense to use the current state (curEventKV) to determine if the TTL has changed during delete?

Thank you for your help!

[brandond]

Perhaps. With other changes from #238 I'm not sure that this block is actually needed any longer.

The DelayingQueue interface does not allow delaying the processing of an entry; the scheduled time for entry to be popped from the queue is only updated if it would cause the entry to be processed SOONER, not later. Ref:
https://github.com/kubernetes/client-go/blob/master/util/workqueue/delaying_queue.go#L347

The intent of these checks is to ensure that the event has not been enqueued with a longer TTL before attempting a delete. handleTTLEvents always retrieves the most recent event from the map before calling deleteTTLEvent on it, and validates the time remaining. If there has somehow been a more recent update to the key made in between when the event is returned by loadTTLEventKV, and when deleteTTLEvent actually attempts to delete the entry, the delete will fail because the resourceVersion has changed. The error path will be taken and the entry re-enqueued, at which point the correct time until expiry will be observed. Given that, I am not sure we need to check the current or previous expiration again here in deleteTTLEvent.

[Yuyz0112]

@brandond Thanks for the clarification! I also think this if-block is not needed anymore while reading the code, maybe leaving it here at this moment is ok. I'm going to close the issue.