Add DeltaFIFO (a controller framework piece) #5473
Conversation
| // KeyOf exposes f's keyFunc, but also detects the key of a Deltas object or | ||
| // DeletedFinalStateUnknown objects. | ||
| func (f *DeltaFIFO) KeyOf(obj interface{}) (string, error) { | ||
| if d, ok := obj.(Deltas); ok { |
There was a problem hiding this comment.
perhaps a type switch here instead of two casts + check?
There was a problem hiding this comment.
nope-- has to be done serially because the deletion marker would be stored as a delta...
|
Part way done, will look more later. |
|
Re-assigning to Brendan since he has already started reviewing (it was gonna take me a while to catch up anyway). |
| func (f *DeltaFIFO) Get(obj interface{}) (item interface{}, exists bool, err error) { | ||
| key, err := f.KeyOf(obj) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("couldn't create key for object: %v", err) |
There was a problem hiding this comment.
same, wrt masking errors
|
Nothing too major. |
|
Will make changes and do another push when I get back next week, couldn't resist arguing a few points though. ;) |
|
|
||
| // DeltaFIFO is like FIFO, but allows you to process deletes. | ||
| // | ||
| // DeltaFIFO solves this use case: |
There was a problem hiding this comment.
More documentation of the model and intended usage is necessary.
There was a problem hiding this comment.
This is basically a producer-consumer workqueue implementation?
|
Mostly fixed. Still thinking about Get/List & thread safety. |
lavalamp
force-pushed
the
fix6
branch
2 times, most recently
from
44ec4e7
to
e5fa695
Compare
|
OK, I pushed a change to make sure that all slices (the Deltas objects) always only have one owner, which should resolve the potential threading problem that @brendandburns noticed. Sorry for the delay, should be ready to go now. |
|
Aside from moving the function up like @bgrant0607 and I requested, LGTM. Defer to Brian for final LGTM that his comments have been addressed. Move the function up and get an LGTM from @bgrant0607 and I'll merge. --brendan |
Test coverage for module at 80%.
|
Huh, I thought I did move that-- Oh, I see, I moved it almost all the way to the top, I thought it was useful to see the type definition first still. I don't care that much, I moved it the rest of the way. PTAL |
|
Travis is a flake, filed #6233 |
Add DeltaFIFO (a controller framework piece)
Splitting off completed chunks from #5270.