fix: using tombstones to account for rapid deletion #2317
Conversation
| synchronized void onEvent(T resource, boolean unknownState) { | ||
| if (!tombstones.isEmpty()) { | ||
| var iter = tombstones.entrySet().iterator(); | ||
| var entry = iter.next(); |
There was a problem hiding this comment.
Not sure if understand this part, it always removes one random event from the tombstone if expired? Didn't you mean to iterate over all events?
There was a problem hiding this comment.
No, this is to amortorize the expiration logic - you could of course decide on a greater number than 1. Since it's a linked hashmap it's not a random entry.
There was a problem hiding this comment.
Maybe desribe in there as a comment too, that is intentional, since I will at least naivly stop on this every time I open it :)
There was a problem hiding this comment.
I would like to see this as a comment, indeed, as well.
...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java
Outdated
Show resolved
Hide resolved
...ava/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java
Show resolved
Hide resolved
| synchronized void onEvent(T resource, boolean unknownState) { | ||
| if (!tombstones.isEmpty()) { | ||
| var iter = tombstones.entrySet().iterator(); | ||
| var entry = iter.next(); |
There was a problem hiding this comment.
I would like to see this as a comment, indeed, as well.
closes: operator-framework#2314 Signed-off-by: Steven Hawkins <shawkins@redhat.com>
|
@metacosm @metacosm updated the code with further comments and consolidated the secondary caching concerns to a single class. From what I can tell it will take around 170 MB to track up to 1000000 deletions. Also updated the base retention time to 10 minutes, rather than 2. Based upon seeing what the retained heap size of this could be, I also updated the knownResourceVersions cache to be able to store more entries. |
closes: #2314
If during a creation the events for both the add and the deletion occur before the resource is being added to the temporary cache, the temporary cache moves backwards in a way that may not resolve itself.
The approach here is to use tombstones to track deletions. The benefit of this approach is that it keeps the logic encapsulated to the TemporaryResourceCache. The drawback is that we're only guessing about how long to retain the tombstones.
The other approach would look more similar to event recording, which is notify the cache that a create is happening prior to when it starts so that deletions / tombstones can gathered for that specific resource.